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

set method creates keys on store itself #64

Closed
tommoor opened this issue Jan 7, 2015 · 6 comments
Closed

set method creates keys on store itself #64

tommoor opened this issue Jan 7, 2015 · 6 comments

Comments

@tommoor
Copy link
Contributor

tommoor commented Jan 7, 2015

Using the set method to set keys that are in the scheme it seems as though the data is set on the store object itself, wouldn't it make more sense to have some sort of data object within the store so that these properties aren't mixed in with other methods?

Maybe I'm missing something...

@darcyadams
Copy link
Collaborator

I agree. Been hoping to get around to this for awhile, but haven't made it happen yet. If you want to take a stab, go for it.
I must say, it is handy to have them as top level attributes when you need to do data manipulation as opposed to setting, but I agree that the potential for accidental conflicts outweighs that minor benefit.

@tommoor
Copy link
Contributor Author

tommoor commented Jan 7, 2015

Okay, glad I'm not going crazy - I also expected there to be some sort of default getState method implemented and I ended up just having to do:

getState: function() {
  return this;
}

which isn't great.

@darcyadams
Copy link
Collaborator

Well, the benefit of using scheme is that the scheme attributes are automatically applied as state in the views with the flux mixin. If you don't use scheme, yes you have to manually define a getState method.

@darcyadams
Copy link
Collaborator

... Actually the need to define the getStae method is what led to the initial idea for the scheme enhancement.

@tommoor
Copy link
Contributor Author

tommoor commented Jan 7, 2015

Got it, this is useful.

I guess the problem is that only a few of our stores are treated like models (one for app state for example). The majority of stores are used as the flux architecture expects - more like a collection of models. This is why I ended up implementing a getState method so that there was a consistent way to access the data across these different types of stores.

Ideally the scheme methodology would work with a collection/array as well and implement a standard method to get as well as set the data imo.

@darcyadams
Copy link
Collaborator

closing as this is now corrected in master

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

2 participants