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

Transact action should support common encoding formats- JSON, Transit, edn, and Form-POST #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ohpauleez
Copy link
Contributor

@ohpauleez ohpauleez commented Mar 24, 2017

This addresses issue #45

Resolving the "payload" parameter for transact is handled by a separate function.
Currently, this function also grabs form params as a last effort (which are not nested under payload).

@ddeaguiar
Copy link
Contributor

Unrelated to this PR but I noticed that the merged-parameters fn doesn't include transit-params. Should it?

@ohpauleez
Copy link
Contributor Author

It does now :) -- nice catch.

@ddeaguiar
Copy link
Contributor

I think we'll want to add form-params to merged-parameters as well. Another thing is that the caller is expecting a collection of maps but :form-params will just be a map. AFAIK, you can't submit a collection.

@ddeaguiar
Copy link
Contributor

@ohpauleez I spent some time thinking about this PR and thought that extracting resolve-payload-parameter to an an interceptor or interceptors may provide more flexibility. I'd consider at least two interceptors here. One for json, transit and edn params and another for form params because they require some transformation, at least based on the current usage in transact-action-exprs. I'd have transact-action-exprs depend on a new params key, like :vase-params, whose contents should be a collection of txdata, which would be populated by these interceptors.

In the future this could even be made configurable so that developers could specify which content-types are supported – a consequence of this is that such information could be feed into specification generation tooling for things like Swagger.

I'd be happy to adopt this PR and hash this out if you're up for it.

@ddeaguiar
Copy link
Contributor

Alternatively, resolve-payload-parameter can stick around and support json, transit and end payloads but :form-param support could be moved to an interceptor. I think a new params key would still be needed in this case, though.

@ddeaguiar
Copy link
Contributor

Ping @ohpauleez just following up. This PR is related to issue #78 as well.

@CalebMacdonaldBlack
Copy link
Contributor

I'd really see this PR make it in to send edn instead of json

@ddeaguiar
Copy link
Contributor

@ohpauleez I'm guessing this work is sitting on the back burner. Would it be ok if I pushed it forward?

@ddeaguiar ddeaguiar added the 0.9.4 Include in 0.9.4 release label May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.9.4 Include in 0.9.4 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants