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

Move to React Context naming #5

Closed
passsy opened this issue Mar 30, 2018 · 15 comments
Closed

Move to React Context naming #5

passsy opened this issue Mar 30, 2018 · 15 comments

Comments

@passsy
Copy link
Contributor

passsy commented Mar 30, 2018

React just published the Context API with the same features as scoped_model. I was thinking about changing the names of scoped_model to conform the React names (Which are pretty good).

ScopedModel<AModel> -> ContextProvider<AModel>
ScopedModelDescendant<AModel> -> ContextConsumer<AModel>

//cc @apwilson

@brianegan
Copy link
Owner

brianegan commented Mar 30, 2018

Hey @passsy,

Totally let me know if I'm wrong here, but I don't think they're quite the same.

  1. Context API != ScopedModel, Context API == InheritedWidget. In order to change the State using the Context API, you need to call setState on the Component that contains the ContextProvider and mutate the component's state. This will rebuild the ContextProvider + Consumers. This is exactly how InheritedWidget works by default. ScopedModel = Context API + Reactive Model. To change state, you update the Model and it notifies the listeners, but it does not rebuild the ScopedModel. Since the two are so similar, it's actually why I've wondered if scoped_model is really necessary...
  2. Context means something specific in Flutter (the BuildContext). Therefore, I don't think it's a good idea to use the same name here, as a newcomer might think "Oh, am I providing or consuming a new BuildContext?"

Whatcha think?

@passsy
Copy link
Contributor Author

passsy commented Mar 30, 2018

We can agree that scoped_model and InheritedWidget are very close together. Especially because scoped_model uses InheritedWidget internally. I still think ScopedModel is a better equivalent to the Context API than InheritedWidget alone.

  1. Models also have to call setState indirectly via notifyListeners() to send updates down the tree.
  2. InheritedWidget alone has no equivalent to ContextConsumer like scoped_model has with ScopedModelDescendant. InheritedWidget implicitly registers a dependency when calling inheritFromWidgetOfExactType which is not as clear as in scoped_model or the Context API. Pretty hard for newcomers.

The distinction between BuildContext and Context could be a problem and the main reason we will not change the name. Although I've never seen somebody creating variables for ScopedModel or ScopedModelDescendant and name clashes with BuildContext will rarely happen.

// never seen something like this
var scopedModel = new ScopedModel(...)
var scopedModelDescendant = new ScopedModelDescendant(...)

What about this?

ScopedModel<AModel> -> ModelProvider<AModel>
ScopedModelDescendant<AModel> -> ModelConsumer<AModel>

@brianegan
Copy link
Owner

brianegan commented Mar 30, 2018

Yep, I think ModelProvider and ModelConsumer are good names and avoid calling too many things "Context"! I always found ScopedModelDescendant to be a mouthful and the name doesn't really describe what it does.

"I still think ScopedModel is a better equivalent to the Context API than InheritedWidget alone." -- From an API standpoint, I agree. From a functionality standpoint, I disagree. But don't think it's worth getting into a long debate. We each view it in a slightly different way :)

I've got a lot going on right now to support Dart 2 for RxDart and a few other libs, would you mind preparing a PR? Also, would be dope to ensure it works for Dart 2 if ya have a chance!

@passsy
Copy link
Contributor Author

passsy commented Mar 30, 2018

will do :)

@brianegan
Copy link
Owner

@passsy Yessss, thanks so much for all the help with this lib! <3

@passsy
Copy link
Contributor Author

passsy commented Mar 30, 2018

I'm undecided. Renaming ScopedModel to ModelProvider would completely vanish the name ScopedModel from this lib. Renaming this lib scoped_model to something else would be the logical next step.

Only renaming ScopedModelDescendant to (Scoped)ModelConsumer is a tiny improvement. Without a ModelProvider it wouldn't reach the goal to match the naming with the Context API. 😩

Requires more thinking...

@brianegan
Copy link
Owner

brianegan commented Mar 30, 2018

Yah, I was thinking about that as well. Overall, I'm comfy with it if you are, but might not be worth it?

🤷‍♀️ 🤷‍♂️

@apwilson
Copy link
Contributor

apwilson commented Apr 5, 2018

After reading this and some discussion with my team we also like a more natural language approach to naming like:

ScopedModel<AModel> =>  Provide<AModel>
ScopedModelDescendant<AModel> => BuildWith<AModel>

Thoughts on that style of naming?

@passsy
Copy link
Contributor Author

passsy commented Apr 5, 2018

I'm fine with Provide and BuildWith. Let's discuss renaming this project separately.

@brianegan
Copy link
Owner

brianegan commented Apr 5, 2018

I'm not the biggest fan of those names (since they're verbs instead of nouns), but I'm happy to defer to others who use this library more often and what feels natural to you.

Please feel free to submit a PR and I can help review :)

@chimon2000
Copy link

Keeping with the theme of mirroring the redux naming patterns like alternatives unistore and react-waterfall, it could be Provider and Connector similar to how flutter_redux has StoreProvider and StoreConnector

@brianegan
Copy link
Owner

Any more thoughts on this? Seemed like we got a lot of suggestions, but not sure if we landed on something. Happy to make this change, just need to know what names you'd like to use :)

@chimon2000
Copy link

I think the consensus seems to be Provide & BuildWith.

@brianegan
Copy link
Owner

brianegan commented Aug 18, 2018

Thanks @chimon2000! @apwilson @passsy -- What are your thoughts? Should I go forward with the Provide / BuildWidth change?

@brianegan
Copy link
Owner

Overall, we're discussing deprecating this package in favor of the provider package. That package uses the react naming conventions: Provider and Consumer. It offers all the functionality of scoped_model, but also allows you to pass down any kind of Value, not just a listenable! I'd recommend moving to that if you'd prefer the react naming conventions.

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

4 participants