goatslacker / alt

Isomorphic flux implementation
http://alt.js.org/
3.45k stars 322 forks source link

Allow setState to be overridden #419

Open bebraw opened 9 years ago

bebraw commented 9 years ago

I'm trying to implement Store level persistency for my application (demo app for a book). The idea is that I restore Store state at constructor and save it whenever it changes. I noticed this.dispatcher.register gets fairly close but unfortunately it's called before state change is applied making it unsuitable for this purpose. As a next step I tried to override setState but this isn't possible due to the current class hierarchy. I believe it would be a good idea to allow this.

Just for the record I ended up "solving" the problem like this:

class NoteStore {
  constructor() {
    this.bindActions(NoteActions);

    this.exportPublicMethods({
      persist: () => storage.set('notes', this.notes)
    });

    const initialData = storage.get('notes');
    this.notes = Array.isArray(initialData) ? initialData : [];
  }
  ...
}

I hit NoteStore.persist() at my store listener. That will trigger the persistency logic.

It is possible there's a simpler way to achieve the same in the current system but I failed to find that. Ideally all of this logic belongs to store level no matter what the solution is.

goatslacker commented 9 years ago

Maybe this?

class NoteStore {
  static config = {
    setState(oldState, nextState) {
      storage.set('notes', nextState.notes)
      return Object.assign(oldState, nextState)
    }
  }

  constructor() {
    this.bindActions(NoteActions);

    this.exportPublicMethods({
      persist: () => 
    });

    const initialData = storage.get('notes');
    this.notes = Array.isArray(initialData) ? initialData : [];
  }
}

Another interesting way of doing this without doing it at the store level and for every store:

// setup a initialize file

const initialData = storage.get('all-the-data');

// all the stores will now have the initial data that we've 
// persisted when the app starts up
alt.bootstrap(initialData);

// now lets set up something to persist the data whenever there is any change
import makeFinalStore from 'alt/utils/makeFinalStore';

const finalStore = makeFinalStore(alt);

// this store gets called whenever all the stores have finished updating
finalStore.listen(() => {
  // lets save a snapshot of all the data so we can reload it later.
  storage.set('all-the-data', alt.takeSnapshot());
});

// want to do more cool stuff with this? setup a websocket 
// and send the snapshot around, bootstrapping the data onto 
// other machines for remote hot loading.
bebraw commented 9 years ago

Thanks! Both solutions are a good step forward.

I thought about this meanwhile. Ideally I would love to have an API like this:

@persist('notes', []) // store data key, default value if it's not found at store
class NoteStore {
  constructor() {
    this.bindActions(NoteActions);
  }
  ...
}

In this case that @persist thinger would load the data from notes and bind it to the object. The API might change a bit but you can see how neat that is. With a decorator like this you have a nice amount of control of what to persist, where and how without leaking detail into stores.

The only problem is that I haven't figured out how to customize constructor behavior through a decorator. I expect setting a static method should be easy, though.

I forgot about FinalStore completely. That's likely the ticket if we can't get the decorator thing to work neatly.

taion commented 9 years ago

As a quick note, I believe this would "just work" with overriding + super with the #344 proposal.

goatslacker commented 9 years ago

@bebraw you also have the init lifecycle method. I think you may be able to add it to the store config and have it copy over into the store, but I forget how to do that. If you can't then I'll accept a PR that lets you do just that.

http://alt.js.org/docs/lifecycleListeners/#init

bebraw commented 9 years ago

Thanks!

FinalStore is clean enough for my purposes so I'll go with that.

Feel free to close this unless you feel setState should become customizable. :+1:

goatslacker commented 9 years ago

setState is already customizable, but I don't think there's a way to bind lifecycle listeners to a store automatically. That I'll be adding.