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

[WIP] MobX 6 support #1569

Merged
merged 6 commits into from
Nov 17, 2020
Merged

[WIP] MobX 6 support #1569

merged 6 commits into from
Nov 17, 2020

Conversation

mweststrate
Copy link
Member

No description provided.

@mweststrate
Copy link
Member Author

mweststrate commented Sep 21, 2020

I think this PR to support MobX 6 is technically completely. However, I don't intent to personally merge and release this PR. I've maintained this project for the last 3.5 years, after building it as demo/PoC for a talk, without ever using it myself in a project (I still totally digg the idea though).

In contrast the project is now being used (with great satisfaction afaik) by many companies large and small. However, the activity in the issue tracker doesn't reflect that.

So if you want to see this project moving forward, please step up and volunteer as maintainer and take this PR through the final steps (once MobX 6 has been released) by testing it against your real life use case and taking care of the remaining steps and follow up issues. I gladly share the necessary permissions.

@bamzi
Copy link

bamzi commented Oct 9, 2020

I'm not qualified to even volunteer as a maintainer but as a fan, and also wanting this PR merged, maybe it's a better idea to update the repo readme file to mention you're looking for maintainers (otherwise this lib may be archived) and link it to this PR or a better write up. Basically any broadcasting efforts, I'm sure, will fare better rather than a relatively hidden PR comment.

@elektronik2k5
Copy link
Contributor

elektronik2k5 commented Oct 9, 2020

I agree and as an avid long time user and advocate of MST feel the same way: I'd like to help, but I don't even know mobx internals enough - let alone MST. Thus I don't feel comfortable stepping up as a maintainer :(.

@kuasha420
Copy link

Maybe the good folks at @infinitered will be interested in this since they are using mst on their ignite-cli.

Tagging @jamonholmgren @rmevans9

@wouterh
Copy link

wouterh commented Oct 12, 2020

I have tested this PR on two different client projects which are currently still on MobX 4. One react and one react-native project. I had to update some calls from ...sort() to ...slice().sort(), but that was it. Overall a smooth upgrade experience.

If I can help with more testing, I will be happy to help.

@makarov-roman
Copy link

Can we create RC release on npm to simplify the testing a bit?

@mweststrate
Copy link
Member Author

mweststrate commented Oct 17, 2020

I don't think there is any reason to be too concerned whether you have the knowledge required. I've maintained it the past few years, but only wrote ~half of it, and never used it in any professional project beyond some consultancy. So I'm pretty sure most people in this thread have more experience with it than me :) Clearly that doesn't bring familiarity with the code itself immediately, but that only build up through experience. And if I look at the issue tracker, a lot of questions are also a how-to...? I think maintaining a thing like this is mostly taking a plunge, and guided by any specific issue you want to address, PR that needs merging, you will become familiar over time. But if anyone wants a specific tour through the code let me know.

If anyone wants to release an RC, feel free to share a NPM username and I'll add publish rights. For me it will be clearly only like ~5 minutes of work, and for anybody else more. But that holds for processing any issue / PR / etc. So this is my explicit hand off point. If you interested in maintaining you're always free to ping me about anything specific, but I won't take the driver seat.

Also I think that most PRs open now can be merged and released without much though and are pretty low-hanging fruit. I suspect the most is true for a lot of issues as well. There is no need to start without anything complicated, the core of this project is pretty solid and can be build upon. The only complicated thing that might need addressing is simplifying the TS types, since TS has become some much more powerful in the last 2 years. But I can probably help with that.

@Bnaya
Copy link
Member

Bnaya commented Oct 17, 2020

Can we create RC release on npm to simplify the testing a bit?

I've released MST 4 & mst-middlewares 4 rc's on the @rc dist tag

npm install mobx-state-tree@rc;
npm install mst-middlewares@rc;
yarn add mobx-state-tree@rc;
yarn add mst-middlewares@rc;

I started working on it just before @mweststrate message.
Worth noting: I'm no longer actively using MST, So I don't have the capacity to do more than that

As an extension to what @mweststrate wrote,
If you are using MST on a daily basis, have a decent typescript skills, and can spare like 10 hours/month, you can be good maintainers for MST.

@yordis
Copy link
Contributor

yordis commented Oct 17, 2020

@mboperator would you mind testing your app with the @rc version for like at least 2 hours?

I am more than happy to pair-programming and help you with the issues.

@kuasha420
Copy link

Thanks for the RC, I'll test it out tomorrow!

@bmichotte
Copy link

Upgraded from 3.17.2 to rc smoothly, no issue so far

@@ -1,7 +1,8 @@
import { types } from "../../src"
import { isObservableProp, isComputedProp } from "mobx"

test("this support", () => {
// MWE: disabled test, `this` isn't supposed to work, and afaik nowhere advertised
test.skip("this support", () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is referenced in the typescript docs

@jamonholmgren
Copy link
Collaborator

Apologies, I missed my tag on this. I’ll have my team test it!

@jamonholmgren
Copy link
Collaborator

So far we've tried it on two projects and other than some useObserver deprecation warnings (which aren't hard to refactor), it seems to be working fine.

IMO, we should merge it and release it, and then fix any edge cases in future patch-level releases.

@jamonholmgren
Copy link
Collaborator

I'm willing to take on ownership of this repo and manage releases. My NPM username is jamonholmgren. @mweststrate @Bnaya.

@mweststrate
Copy link
Member Author

@jamonholmgren done, thanks!

@jamonholmgren
Copy link
Collaborator

My first act as Supreme Commander will be to rename it jamon-state-tree. I don't think anyone will mind.

@RainerAtSpirit
Copy link
Contributor

@jamonholmgren You bet we will ;-).

@radiosilence
Copy link

@jamonholmgren as long as it supports Jamon 6.0 !

@jamonholmgren
Copy link
Collaborator

I will be releasing mobx-state-tree@v4.0.0 shortly. Cross your fingers and say a prayer. 😄

[ooh, I love how fast the CI finishes on this project!]

@jamonholmgren jamonholmgren merged commit 02dcdf2 into master Nov 17, 2020
@jamonholmgren jamonholmgren deleted the mobx6 branch November 17, 2020 03:31
@mikekrell
Copy link

@jamonholmgren You are the MAAAAN! How do I buy you a beer?! And the Portland Trail Blazers are the best team in the NBA.

@kuasha420
Copy link

mobx-state-tree@v4.0.0

You mean jamon-state-tree right? Seriously though, thanks for your efforts here. I feel like a great matchmaker for tagging you.

I'm working on a tslint -> eslint migration for this library and hope to push out the PR soon.

@cannc4
Copy link

cannc4 commented Nov 17, 2020

Thanks @jamonholmgren !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help/PR welcome Help/Pull request from contributors to fix the issue is welcome
Projects
None yet
Development

Successfully merging this pull request may close these issues.