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

Revision of with-mobx example #3260

Closed

Conversation

michaelsbradleyjr
Copy link

This pull request is not ready to merge; rather, it's a request for feedback and suggestions on how to improve integration of MobX with Next.js. See #3206.

@ghost
Copy link

ghost commented Nov 15, 2017

Hi @michaelsbradleyjr !

Your withMobx higher order component is exactly what I was looking for. I've been dabbling around with next and mobx and couldn't find a good way to integrate the store into the getInitialProps.

I haven't tried your example yet but it sounds very promising.

Watch out for the linting errors. The CI fails because of that. Use Standard style.

Cheers 🍻

@timneutkens
Copy link
Member

Try running npm run lint -- --fix to fix the standard style 👍

@timneutkens
Copy link
Member

@michaelsbradleyjr can you check the above 😄

@michaelsbradleyjr
Copy link
Author

michaelsbradleyjr commented Nov 23, 2017 via email

@michaelsbradleyjr
Copy link
Author

michaelsbradleyjr commented Nov 26, 2017

I ran npm run-script lint -- fix (had to --fix it manually with standard cmd also, for some reason), commited the changes, and then rebased against zeit's canary branch after fetching its latest commits.

To be honest, I'm not sure this is ready to be merged. It was/is more of a proof of concept, and I was hoping to get a little more feedback. I'm wondering if I should extract the withMobx function, genericize it a bit further, include it in a separate package à la next-redux-wrapper (probably named next-mobx-wrapper), and then revise my modifications on this branch to make use of that package. Also, my modifications to the examples are a bit clumsy, even if they do convey different aspects of withMobx usage. If anyone has some suggestions on improving the modified examples, I'm all ears.

@zenflow
Copy link
Contributor

zenflow commented Nov 26, 2017

@michaelsbradleyjr I'll see if I can get a chance to check this out soon

@timneutkens
Copy link
Member

timneutkens commented Feb 4, 2018

Lets keep the example simple for now 🙏 Thank you very much for your contribution!

@timneutkens timneutkens closed this Feb 4, 2018
@joshuaquek
Copy link

Agreed, I actually prefer @michaelsbradleyjr revised MobX example over the one now on the canary branch

@adavia
Copy link

adavia commented Jun 7, 2018

Adopting the withMobx wrapper by @michaelsbradleyjr seems to be the best way to integrate mobx with next.
Any idea why i get this warning: "MobX Provider: Provided store 'store' has changed. Please avoid replacing stores as the change might not propagate to all children"

This shows up during HMR or while navigating between routes.

@michaelsbradleyjr
Copy link
Author

michaelsbradleyjr commented Jun 7, 2018

It's been some time since I looked at this... quite a few things came up in my professional life and my time was diverted away from next.js and MobX. Tomorrow, I'll look at creating a standalone next-mobx-wrapper package. Also, my adaptation of the with-redux-wrapper examples, demo'ing the MobX wrapper's usage, were really just proof-of-concept and quite muddy — I think I can do a lot better. I'll work on all that and put together a with-mobx-wrapper branch and pull request.

@adavia
Copy link

adavia commented Jun 7, 2018

@michaelsbradleyjr sounds great!

@michaelsbradleyjr
Copy link
Author

Working on it: https://github.com/michaelsbradleyjr/next-mobx-wrapper

Converting the proof-of-concept into a package with good ergonomics has proved to be an interesting project.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants