Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ring-handler clobbers :params #21

Open
patrickthebold opened this issue Oct 5, 2015 · 5 comments
Open

ring-handler clobbers :params #21

patrickthebold opened this issue Oct 5, 2015 · 5 comments

Comments

@patrickthebold
Copy link

It seems that if one uses the standard ring middle-ware 'wrap-params', that map gets lost when using ring-handler. I was thinking we could either merge the :params map provided by wrap-params with the one provided by silk, or put the silk params under a different key. Thoughts?

@Deraen
Copy link
Collaborator

Deraen commented Oct 6, 2015

The usual solution is to merge params to :params and add them under another key, e.g. :path-params, :query-params.

@domkm
Copy link
Owner

domkm commented Oct 7, 2015

Silk can extract params from the query. Are you sure you need wrap-params? wrap-params can also extract form params but that is available under the form-params which is not clobbered by Silk. If you need wrap-params, I think you should follow @Deraen's suggestion, but I would first evaluate whether you really need wrap-params.

@patrickthebold
Copy link
Author

I'm using sente which requires wrap-params and wrap-keyword-params. Of course, I can work around it, but I think the "right thing to do" would be if silk didn't clobber the params. That way it would play nicer with other libraries. Would you accept a PR that merges the :params and adds the silk ones under, say, :route-params?

@Deraen
Copy link
Collaborator

Deraen commented Oct 8, 2015

Btw. wrap-params doesn't only read query string parameters but also form parameters from request body, so it's quite common to use it.

@domkm
Copy link
Owner

domkm commented Oct 9, 2015

Thanks for the clarification. I'd accept a PR that merges Silk params into the request params instead of displacing them. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants