class extends Store

[goatslacker]

Hey, it's a start.

[goatslacker]

Not super happy with this, but I can live with just forcing passing the flux instance into super. Not the config though, I'd love to be able to pull that from static

[taion]

If you already know that this is a Store (from the base class), maybe something like an @app.register decorator to connect it to the app and return a singleton? Basically the same thing as alt.createStore, but without repeating the "store" part in the name.

[taion]

Is registerAsync gone now? Now that there's a base class, I think it might be possible to do make data-source-behavior be a method on the base Store class, so for the http://alt.js.org/docs/async/ example, something like

class SearchStore extends Store {
  performSearch() {
    return this.datasource({remote, local, loading, success, error);
  }
}

which I think is a little bit less boilerplate-y and more explicit than defining a separate helper object.

Though that said, take a look at http://martyjs.org/guides/fetching-state/index.html and https://github.com/martyjs/marty-lib/blob/master/src/store/storeFetch.js. This is partially analogous to the data sources here - I think the big differences are

1. Marty Store#fetch calls are required to have an ID, and we keep track of the pending fetches (by ID), so multiple fetches on the same ID don't trigger repeated calls to remote
2. Marty Store#fetch doesn't automatically set up an action creator for dispatching actions (i.e. the Alt data source support for loading/success/error actions.
3. Marty Store#fetch returns a special "fetch" object. Marty containers wire up individual props on the contained component to values on the store, and part of this mapping knows how to handle those special objects, so from the POV of a user writing a container, a getter returning this.fetch behaves as if it were just returning the value.

I don't know if you think this is too opinionated - I think the abstraction there is fairly useful, because mostly this is what I do want for read operations on stores that may need to hit up a remote. But that said, the handling of pending/failed fetches can be improved, per martyjs/marty#346

[taion]

Probably need to keep this around with a deprecation warning to make it easier for users to upgrade.

[goatslacker]

Not bad, ended up removing more code than I added...I like this direction.

I only wish I could keep the current API as well so there aren't any breaking changes. I don't see how that's possible though since I'd need to dynamically extend your class -- I don't think JS can do this with ES6 classes like they could with prototypes.

[taion]

You sort of can... walk up the prototype chain of the class passed in and use setPrototypeOf.

I think net, though, saying e.g. alt.register(class extends Store { ... }) is not too much more verbose than alt.createStore(class { ... }), and it can be helpful to tie legacy behavior to the latter to ease migration.

[goatslacker]

It's not terrible and it works just fine, its just that if we can have no breaking changes then that would be a huge plus.

[taion]

Are you saying to keep the createStore API around indefinitely, or to just not immediately break users on update (until a future major release)?

For the latter, I think I'd structure the API as with #register(), #_registerStore(), and #createStore(). The #register() method could use instanceof to check what kind of component is getting registered, but everything else can just use duck typing, and then #createStore can just attach the base Store class methods to the prototype of what's handed in. Or actually just extend the passed-in FooStore class and add the relevant new methods to the prototype of the extended class.

For the former, well, I'm resigned to eventually having to re-write my code post-underdash. I'd just like to be able to rewrite it piece by piece and just use deprecated compat wrappers until the next big release afterward.

[goatslacker]

In the strict definition of indefinite then yes, but I'm not thinking about that right now. The less breaking changes for all the better though, specially if it means very little code added. We're already in the negative in LOC (which is great) with this change.

The register method already uses instanceof check btw.

[taion]

Oh, didn't see the new commits.

In which case why not just call into the private registerStore from createStore after attaching the relevant new methods to e.g. a new subclass or something? You can duck type it everywhere except for Application#register, no?

[taion]

Something like

createStore(OldStyleStore) {
  class WrappedStore extends OldStyleStore {
    constructor() {
      super();
      this::Store.prototype.init();
    }
  }

  Object.getOwnPropertyNames(Store.prototype).forEach(function extendStore(propertyName) {
    if (propertyName === 'constructor') {
      return;
    }

    WrappedStore[propertyName] = Store.prototype[propertyName];
  });

  this._registerStore(WrappedStore);
}

[taion]

Does this work if StoreModel extends another class? Looks like getInternalMethods might have to walk up the prototype chain for StoreModel for this to work.

I think it'd be easier just to pull out the constructor functionality on the base Store class into an #init() method, then invoke that (in both Store#constructor and for this), without messing with StoreModel's prototype.

[goatslacker]

Nah it doesn't it should walk up the proto chain.

I doubt this will work with proper ES6 classes anyway, I may just nuke this. This was just a fun experiment :)

[taion]

FWIW my thought with using the base Store effectively as a mixin is that you fully control that, so you know that it doesn't have any superclasses (or if it does, you know how they're set up), whereas you can't assume anything about whatever crazy thing the user passes in to createStore.

[goatslacker]

Got it. Just updated with the latest. I don't think this will work with ES6 classes though (not transpiled) I'll have to verify.

Also, I don't really want to export the whole store instance. Does this make the benefits of extends useless?

I like this direction btw, it removes a lot of code.

[taion]

Are proper ES6 classes that static? I just assumed that Babel was doing in some sense the right thing.

I drafted it up with extending the supplied StoreModel just in case users had their store models extend other classes, or were doing instanceof checks or whatever. Outside of #register() you ought to be able to just duck type all the stores, so it seemed "nicer" to apply a store mixin on top rather than change their hierarchy.

[goatslacker]

benefits of doing this:

• Really small surface API layer for stores.
• Does not leak methods unless you explicitly do so.
• Does not export setState!

[taion]

Also that it doesn't dramatically change behavior for existing users :p

There's the flip side to this, though, which would be to hide methods that are bound as action handlers, and to hide methods we know to be unsafe/private-only, and reveal other methods by default. This would be more work and more complicated at the library level, but in return

• Less boilerplate for people who like adding public getter methods to stores
• If running in non-production mode, you know what all the hidden methods are, so you can emit a useful error message if users attempt to use them

Not sure which is better - tagging all getter methods with @expose is a bit weird - I can see the benefit of keeping users from calling e.g. internal-only helper methods, but you might get most of the benefit if you just hide methods that handle actions.

This is really w/r/t #361 - there's no good reason to change the behavior now.

[goatslacker]

That's smart. If it yields less LOC I'm all for it although I think I might give a slight edge to the current behavior because it's explicit what you expose whereas hiding automatically feels a bit magical.

[taion]

I think both behaviors are a little bit more magical. I think the bigger trade-off is that hide-by-default makes it impossible to unintentionally expose a method, but show-by-default reduces boilerplate when writing getters.

[goatslacker]

If we do show by default we'd have to implement a @Private (or equivalent) decorator which would hide certain methods.

[taion]

That makes sense.

I guess my question is - what's the reason behind hiding the store methods in the first place?

Is it mostly a learning aid to keep inexperienced users from doing inappropriate things with stores? In that case, just hiding the handlers and setState should mostly get the point across (I hope).

If it's to allow store API designers more fine-grained control over method visibility, I feel like there are better and more idiomatic ways to do that than to implement support in a Flux library. I could always just name those functions with non-exported ES6 symbols or use other more generic patterns for private methods.

[goatslacker]

It's a bit of both. This is something that's appreciated at a few companies so people can't just FooStore.setState() or FooStore.internalMethodCall()

[taion]

Got it - and that makes a lot of sense across a larger dev team. And I'll probably have the same concerns since I'm still trying to write a library that generates stores for the user.

What strikes me as a bit odd is that it's implemented as a special thing on Alt stores, though. I agree that it's handy to have private methods, but it seems a little strange for it to be particular to these stores. The private method thing seems like it should be handled in a way that's more generic than that. This is definitely one of the places where it comes up the most keenly, though.

[goatslacker]

One thing I'm going to dearly miss:

class Foo {
  constructor() {
    this.lol = 1

    this.bindAction(actions.increment, () => {
      this.lol = this.lol + 1
    })
  }
}

[taion]

Is this because everything now has to go onto store.state instead of being assigned to properties of the store object directly?

I guess I don't fully understand what the old code does when you do that - is it setting the lol property on some object other than the store object?

[goatslacker]

It sets it on the store, but that in turn becomes "state"

With the current implementation I can't get away with this because the instance variables contain other things like "transmitter", "alt", "dispatcher", etc...

[goatslacker]

I could probably hack a way to get around this...

[taion]

If you use the trick where you extend the given store class and assign those to this prototype of the subclass to allow e.g. using this.dispatcher in the constructor, then that shouldn't be an issue, no?

[goatslacker]

I'd have to do that for every store passed in. I don't envision what that looks like now (in a clean way at least)

[taion]

Aren't you already doing this right now, though?

alt/src/alt/store/index.js

Line 38 in 021fbc6

dispatcher: alt.dispatcher,

I think it's a little nicer to subclass and modify the subclass prototype, but if you don't want the extra class, extending the prototype should be fine too.

[taion]

This interesting discussion came up on one of the Marty issues - martyjs/marty-lib#24 (comment)

Essentially we don't have the ability to reset store state when using canonical ES6-class-based stores that set initial state in constructor. I initially suggested something like what Alt does, but it does seem like creating a new store would be a cleaner way to implement this with fewer edge cases than taking an initial snapshot.