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

Minimal cross-framework standard #4

Closed
taion opened this issue Jul 3, 2015 · 30 comments
Closed

Minimal cross-framework standard #4

taion opened this issue Jul 3, 2015 · 30 comments

Comments

@taion
Copy link

taion commented Jul 3, 2015

Continuation of our discussion from Reactiflux -

I like what you're trying to do here, but I think what you have maps more closely to what you've built in Flummox than you intend, and doesn't do a good job of describing how other Flux frameworks behave, which limits its utility as a potential interop standard and makes the name not as descriptive as it could be.

For example, you specifically privilege status, while it's not at all standard for Flux frameworks to have a concept of action status as separate from the action itself - neither Alt nor Marty have specific concepts of success/failure actions and just have those treated as actions with different constants.

As I see it, there are really 3 types of data on an action -

  1. Type/constant: identifies the relevant handler for the action
  2. Payload: user-specified contents of the action
  3. Metadata: framework-level information regarding action

I'd probably organize the spec rather like:

{
  type,
  payload,
  metadata
}

I'd specify:

  • type: Something potentially appropriate as an object key, either a name string, a mangled name/ID string, or an ES6 Symbol - this should be sufficient to uniquely set up handlers
  • metadata: Framework-specific values not generated directly by the user - these can be things like original name of the action (in case it was mangled to dedupe), semantic action status for frameworks that explicitly model this (again, most frameworks do not), or other logging/diagnostic flags, or things useful for middleware
  • payload: As close as possible to what the user directly specified

I can see where you're going with specifying status as a special thing, but for my tastes, it's too opinionated. I understand that you feel that it makes sense to Flux frameworks to have semantic notions of action status, and I can see the arguments you've made in that regard, but it's not suitable to insert this as part of what aims to be a "minimal, common standard" when other frameworks often do not express or capture that notion in this way - and moreover when there exists a more generic abstraction (i.e. the metadata) that can capture similar framework peculiar things in a way that makes it clear where the potential interop barriers sit.

@clearjs
Copy link

clearjs commented Jul 3, 2015

Or limit scope to Redux, so that it doesn't get too generic. Probably most middleware wouldn't work with other frameworks anyway.

@acdlite
Copy link
Contributor

acdlite commented Jul 3, 2015

@taion Every dispatched Flux action is part of a asynchronous sequence. Think of every other kind of asynchronous sequence — observables, streams, promises, etc. *All * of them have the concept of being in a "error" state.

  • Observables have onNext() and onError()
  • Promises can be in either a "resolved" state or a "rejected" state.
  • Streams have on('error')
  • Even Node-style callbacks have (err, x) => {...}

status is the solution I came up with for this initial version of the standard. Its sole purpose is to indicate if action represents an error, without requiring knowledge of different, specific action types.

Yes, you're right that current Flux libraries often use separate types to implement error handling — they can continue doing that while still conforming to FSA, simply by omitting status entirely. status is a totally optional part of the spec, though also very useful one that increases interop between tools.

Here's an exercise: look at the source of redux-promise and tell me how you would implement it with multiple action types. https://github.com/acdlite/redux-promise/blob/master/src/index.js#L15-L20

If the word "success" is too confusing, we can rename it to "next" using the Observable parlance. Perhaps that would be clearer.

A metadata field (@goatslacker suggested the same thing) is worth considering, but I'm not sure if it's worth it... It seems just as good to put metadata in the top-level state object. But we can open a separate issue for that.

@acdlite
Copy link
Contributor

acdlite commented Jul 3, 2015

@clearjs There's no reason why a spec that works for Redux shouldn't also work for all Flux implementations. Even middleware isn't Redux-specific. All middleware does is wrap a dispatch() method, so they work with every Flux library that has one of those. Hint: all of them :)

@taion
Copy link
Author

taion commented Jul 3, 2015

I don't think status improves interop because it specifically highlights an optional semantic field without really signposting that, outside of e.g. Flummox and Redux, it's not something special or with semantic meaning.

In my view, the benefit of putting status inside something like a broader metadata is that it explicitly signposts that you have something where the semantics are not universal. If you use that middleware and then pass the action to a store handler that (by design) just looks at the payload, what do you get? Nothing good/correct. You end up needing another piece of middleware-like behavior on the handler end to resolve how to handle the action from something other than the type and the payload. In other words, you end up with a framework-specific piece of metadata, that might as well be captured under a broader notion, rather than as as a unique and special status field.

Hence what I have outlined - type and payload are the bare minimum for dispatching and handling actions, and a minimal standard should aim to be agnostic toward anything else. Implementation-wise, success/failure status seems like a special case of "metadata meaningful to framework framework or specific piece of middleware used", which seems like a generic, useful, universal construct that most Flux frameworks make some use of.

EDIT: Fixed phrasing above

@clearjs
Copy link

clearjs commented Jul 3, 2015

@taion What value would such generic standard provide? Which kinds of tools could be built on it?

@taion
Copy link
Author

taion commented Jul 3, 2015

Also, while I can parse this on a technical level, in plain English this makes no sense if I'm dispatching a GET_USER_FAILED action:

status MAY be undefined, in which case the consumer MUST treat the action as if its status were success.

But mostly my point is that I look at status as being a sub-case of metadata, and that a standard should include the more general thing when it's useful, which metadata would be.

@taion
Copy link
Author

taion commented Jul 3, 2015

@clearjs

It gives you a way to dispatch actions from one framework that another framework can use.

And standardizing all information other than the type and payload under a meta field gives a reasonable place where potentially interoperable middlewares can stash their relevant keys/data.

@clearjs
Copy link

clearjs commented Jul 3, 2015

Metadata can be defined as every field except payload, no need to nest it IMO.

@acdlite
Copy link
Contributor

acdlite commented Jul 3, 2015

outside of e.g. Flummox and Redux, it's not something special or with semantic meaning

Not currently, but it should! Good standards encourage good ideas. Making failed actions (actions that represents errors) a first-class concept is important, for the reasons described in my comment above. Suggestions for alternatives are totally welcome, but omitting it completely is not an option.

I'm thinking we should replace status with a boolean error property (as I believe @clearjs suggested) to make it clearer what its purpose is. Also cleaner, since there are only two valid values anyway.

@clearjs
Copy link

clearjs commented Jul 3, 2015

@acdlite A boolean is one option. An error object (e.g., exception) is another. In the latter case, payload could contain user-friendly error message.

@taion
Copy link
Author

taion commented Jul 3, 2015

Usually you start with the ideas, not with the standards...

If meta.error is used everywhere to convey error status, why is that worse than having error be a top-level optional boolean field?

Isn't success/error status literally metadata here?

@taion
Copy link
Author

taion commented Jul 3, 2015

One thing I might very well want to do is have a meta.status field with enum values for starting, done, and failed. This maps much more closely to what I generally want to do with async actions than just success or error. At some level a special top-level standard-defined status or error field precludes or makes it much more awkward to do this, which seems like an anti-feature.

@clearjs
Copy link

clearjs commented Jul 3, 2015

Isn't success/error status literally metadata here?

@taion I think it is, in the same way as action type may be considered metadata.

@acdlite I wouldn't define what the error field contains in the spec, except stating that any falsy value would correspond to success, and truthy to error.

@acdlite
Copy link
Contributor

acdlite commented Jul 3, 2015

Usually you start with the ideas, not with the standards

This isn't some idea I pulled out of my ass yesterday. Flummox has been using it successfully for months, and it's worked really well. If you give me a specific reason why treating errors as a first class concept is a bad idea besides "other libraries don't do this" I will take that into consideration.

@acdlite
Copy link
Contributor

acdlite commented Jul 3, 2015

@taion starting and done are very different from failed. The first two are about time and the second is about errors. starting and done should be metadata.

@taion
Copy link
Author

taion commented Jul 3, 2015

@clearjs

I think type is special because it's the minimum for meaningfully dispatching an action.

@acdlite

But they'd be used in a very similar way, e.g. with something like a promise-based middleware that dispatched starting at the beginning and then either done or failed at the resolution or rejection of the promise, and then hooks to handle this from stores (e.g. when building higher-level abstractions around fetching data).

I think the sheer fact that we can't agree over how a status field ought to be used is prima facie evidence that it's not an obvious fit for the standard. I just think it's better to go from the perspective of adoption driving a standard than the standard attempting to drive adoption - otherwise if people find something better, you're just left with cruft in the standard you have to deprecate.

But in any case it's not a "minimal, common standard" if you have a carve-out for something that only Flummox does, regardless of the inherent merit of the feature.

@clearjs
Copy link

clearjs commented Jul 3, 2015

@taion Yes, it is special. Since it is necessary, it is a required metadata field, not optional.

@acdlite
Copy link
Contributor

acdlite commented Jul 3, 2015

But in any case it's not a "minimal, common standard" if you have a carve-out for something that only Flummox does, regardless of the inherent merit of the feature.

@taion I simply disagree. If something's a good idea, we shouldn't leave it out of the spec just because it's not yet popular.

@goatslacker goatslacker mentioned this issue Jul 5, 2015
@goatslacker
Copy link

Bringing over the latest comments from #8

{
  // any: Symbol, string, number, object, function. A unique way to identify the action dispatched
  type: ActionTypes.ADD_TODO,

  // any: whatever is being dispatched
  payload: { text: 'Create FSA Proposal' },

    // information about the action itself
  meta: {
    // string | number: a unique identifier for the dispatch
    id: 'abcdef123',

    // string representation of the particular action type
    // since Symbols are not strings and action type can be anything, having a way to identify an
    // action as a string is useful for serializing, debugging, and logging.
    displayType: 'ActionTypes.ADD_TODO'
  }
}

I don't like the name displayType, other than that I think this is the most minimal interoperable action we can standardize on.

Regarding the debate on here about status or error being first-class I don't really have any strong opinion. You both make good arguments. If I were to lean one way it would be to omit it from the spec because it's not minimal, it's a bit opinionated, and we can't seem to agree on it. But I'm not adamant about it.

@clearjs
Copy link

clearjs commented Jul 5, 2015

Unique identifier for a dispatch seems to be very useful, thus moving it out of meta might make sense. Or it really belongs to a spec extension?

Allowing the type property to be of any type seems controversial. What are the use cases where it would be necessary or what are the advantages?

As for the displayType, why not require type.toString() to represent it instead? It should be possible to create define toString on the type instance or prototype (e.g., for functions). In most cases, this should be unnecessary. This approach might be a good compromise if type is allowed to be anything.

Also, meta shouldn't contain any fields that are required. If and only if a field is defined by the FSA spec core, it must be a top-level field. This way, standard extensions go to meta, and the standard itself may be easily extended in the future (because any non-standard fields at the top level are disallowed and all standard fields are at the top level).

status was really controversial, it's great that it has been dropped. As for error (or isError / hasError?), while not yet commonly accepted practice, it seems to bring a lot of value. For example, it makes it easy to define actions using middleware like https://github.com/acdlite/redux-promise/blob/master/src/index.js#L15-L20

@tappleby
Copy link

tappleby commented Jul 5, 2015

I agree an optional unique identifier would be useful, especially for something like #7. I can see some value in it being a top level field (less typing, avoids needing to check if meta exists), that being said the id is likely generated by the framework/middleware which fits the description framework-level information regarding action.

type being any value does complicate things, in order to cover all the use cases I think requiring strict equivalence (===) is as restrictive as we can be. Being able to serialize actions does allow for some powerful techniques such as: Time Travel, Flux over the wire, Sharing state (this is more about state, but I could see an implementation that allows the same workflow with actions). Perhaps a spec extension could exist that places restrictions on actions that can be serialized?

@taion
Copy link
Author

taion commented Jul 6, 2015

@clearjs / @acdlite

I remain in disagreement over error, which is essentially the same thing as status by a different name. My intent for bringing up the meta field was that this could be where you define all the specific bits for middleware-based routing. You don't need a top-level error-field to enable this, and I think this would be a better standard if it were less opinionated in that regard, and more minimal if it didn't have the unnecessary feature whose benefits could be accomplished all the same under meta.

But I've said my piece about this repeatedly. There's no definition of "minimal" with which I am familiar that includes:

If something's a good idea, we shouldn't leave it out of the spec just because it's not yet popular.

Goodness-of-idea is an orthogonal concept to minimalism. My understanding of the term calls for preferring to not including things that are not strictly necessary, regardless of merit as a good idea or not.

@goatslacker

I like the idea of having specific pre-defined keys under meta to allow mutual comprehensibility when necessary. How would you word those field definitions - with SHOULD or with MUST (if present)?

@acdlite
Copy link
Contributor

acdlite commented Jul 6, 2015

Can you please explain why you don't like error instead of repeating that you don't like it over and over? I've yet to hear a reason from you other than "it's not necessary," which is... not helpful.

On Sun, Jul 5, 2015 at 5:23 PM, Jimmy Jia [email protected]
wrote:

@clearjs / @acdlite
I remain in disagreement over error, which is essentially the same thing as status by a different name. My intent for bringing up the meta field was that this could be where you define all the specific bits for middleware-based routing. You don't need a top-level error-field to enable this, and I think this would be a better standard if it were less opinionated in that regard, and more minimal if it didn't have the unnecessary feature whose benefits could be accomplished all the same under meta.
But I've said my piece about this repeatedly. There's no definition of "minimal" with which I am familiar that includes:

If something's a good idea, we shouldn't leave it out of the spec just because it's not yet popular.
Goodness-of-idea is an orthogonal concept to minimalism. My understanding of the term calls for preferring to not including things that are not strictly necessary, regardless of merit as a good idea or not.
@goatslacker

I like the idea of having specific pre-defined keys under meta to allow mutual comprehensibility when necessary. How would you word those field definitions - with SHOULD or with MUST (if present)?

Reply to this email directly or view it on GitHub:
#4 (comment)

@taion
Copy link
Author

taion commented Jul 6, 2015

@acdlite

I think its not being necessary is a big part of why it should not be included. Like with id and displayType, it fits quite naturally into the proposed meta field, where its placement would allow all the things relating to routing with your middleware per https://github.com/acdlite/redux-promise/blob/master/src/index.js#L15-L20 as discussed.

Maybe this gets to the heart of the disagreement? In your own words, this is supposed to be "a minimal, common standard" - if meta.error is sufficient, why have error? Or alternatively why not bring things like id and displayType up to the top level? To me it's the same kind of optional field - potentially relevant and very useful in certain contexts, but not in all of them.

I'm not arguing that error is necessarily a bad idea - in the same sense that I think things like id and displayType can often be good ideas. What I'm saying is that there's no compelling reason for it to be a top-level error field rather than a meta.error field; the only thing you accomplish by doing the former is to inject a portion of opinionatedness into a standard that would be better served by not being unnecessarily opinionated.

@acdlite
Copy link
Contributor

acdlite commented Jul 6, 2015

I see where you're coming from. My problem with putting it in meta is that I think that should be reserved for things that are totally not defined in the spec at all. I think there should be an "in between" area where things aren't required or part of the core spec, but are optional extensions that can be used if desired, and those things should go on the top level. I'm thinking of error but also of displayType or perhaps a field for optimistic updates if we ever think of a good solution. If we put everything except type and payload inside meta then it kinda defeats the purpose of having a spec, in my opinion. It also cuts against another goal of FSA, which is it should be easy to construct and consume actions by hand.

So how do you like that approach — the core spec is just { type, payload, meta } but we can define optional extensions to the spec for different problem areas, like error handling.

On Sun, Jul 5, 2015 at 5:34 PM, Jimmy Jia [email protected]
wrote:

@acdlite
I think its not being necessary is a big part of why it should not be included. Like with id and displayType, it fits quite naturally into the proposed meta field, where its placement would allow all the things relating to routing with your middleware per https://github.com/acdlite/redux-promise/blob/master/src/index.js#L15-L20 as discussed.
Maybe this gets to the heart of the disagreement? In your own words, this is supposed to be "a minimal, common standard" - if meta.error is sufficient, why have error? Or alternatively why not bring things like id and displayType up to the top level? To me it's the same kind of optional field - potentially relevant and very useful in certain contexts, but not in all of them.

I'm not arguing that error is necessarily a bad idea - in the same sense that I think things like id and displayType can often be good ideas. What I'm saying is that there's no compelling reason for it to be a top-level error field rather than a meta.error field; the only thing you accomplish by doing so is by injecting a portion of opinionatedness into a standard that would be better served by not being unnecessarily opinionated.

Reply to this email directly or view it on GitHub:
#4 (comment)

@taion
Copy link
Author

taion commented Jul 6, 2015

I like that a lot. And I like the idea of formal extensions putting things outside of meta to make life easier - shall we open a new issue to discuss how we handle extensions and what we want those extensions to be?

@clearjs
Copy link

clearjs commented Jul 6, 2015

Spec versioning (actually, extensibility) was the only compelling reason for me to make meta field part of the spec. Otherwise, it wouldn't satisfy the heuristic criteria of spec being minimal - the same goals would be possible to achieve without it in a simpler way.

Error handling seems to belong to the core of spec, since probably every real-world use of actions assumes error handling. Without it, the only spec value is to allow extensions, I guess. Or am I missing something?

Also, without it each user would need to define their own way to handle errors (or select among popular ones), thus diminishing possibility to reuse any middleware that depends on error handling.

When we're talking about action interoperability, I believe this actually means ability to reuse various kinds of code for processing actions: creating them with minimal boilerplate, creating them in a way that solves certain commonly occurring problems, transforming them (e.g., filtering, mapping, reducing).

Even if the only thing this spec enables is to create such reusable libraries, then it is worth the effort. Arguments over merits of particular spec aspects should appeal to specific examples of code that wouldn't work or would work worse than with alternative approaches.

Spec should probably focus more on explaining the goals and value of itself, with examples or references to working code. A series of libraries that @acdlite is creating along with this spec shows its usability (such libraries wouldn't be applicable in projects that follow different action schemas).

@taion
Copy link
Author

taion commented Jul 6, 2015

@clearjs

Like I said, I don't care too much about whether well-defined extensions go into meta. I feel like it's a bit neater to have users put their extra information that's not so well-defined under meta rather than leave it all over the place and have potential key conflicts.

Even without error handling, just by defining exactly what type and payload are already gives us a minimum standard of interop, in that different frameworks will be able to consume actions generated by other frameworks, if they all understand the standard.

Of course a lot of great things can be done with extensions, but I think that's exactly why extensibility is so important. Time travel, optimistic updates, &c. - these are all super cool things that we want the ability to do in a consistent way so that we don't end up needlessly reinventing them over and over again.

@goatslacker
Copy link

Can we close this out with a resolution? I'd like to implement FSA for next alt release.

My current thoughts:

id unique dispatch id.
type string. uniqued for each action.
payload any.
meta object.
error boolean.

@taion
Copy link
Author

taion commented Nov 26, 2015

Well, I'm not using Flux any more, and really the concerns here are probably not that relevant any more. Redux is Redux, Alt is hanging around because Alt is cool, and Reflux remains vaguely undead. I don't think there's anything further productive to accomplish here.

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

No branches or pull requests

5 participants