-
Notifications
You must be signed in to change notification settings - Fork 142
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
Should we add a meta field? #5
Comments
As I mentioned in #4, I think there should be a |
Metadata can be defined as every field except payload, no need to nest it IMO. At least, I don't see any advantages, while making actions harder to comprehend. |
http://www.enterpriseintegrationpatterns.com/EnvelopeWrapper.html An action can be considered an envelope for user-defined payload. |
I think it's clearer to specifically signpost "metadata" as such, though - I agree that it gives you a little more to look at, but it makes it more clear on inspection what is what, and I think it's a bit neater to have a specific keyed place for everything, rather than saying "put whatever else you want at top level". This is how JSON API does it: http://jsonapi.org/format/#document-top-level, and I like it in that context. |
OK, so that's mostly about preferences? A standard might simply work with both options: since both 'metadata' and 'type' are top-level fields, they both would be considered metadata under the definition that I proposed. So, both of the following would be FSA-compliant: {
type: 'DO_SMTH',
payload: {},
metadata: {
field1: null
}
} and {
type: 'DO_SMTH',
payload: {},
field1: null
} Without loss of any functionality already discussed. (Spec extensions which specify additional metadata fields wouldn't, though.) |
I see both arguments here, but I'm leaning toward having a separate field because it clearly distinguishes between standard and non-standard properties. |
Added in v0.6.0 |
This would keep the top-level action object predictable — 1 required property (
type
), and 3 optional properties. All other properties at the top-level would be invalid.@goatslacker @taion
The text was updated successfully, but these errors were encountered: