-
Notifications
You must be signed in to change notification settings - Fork 642
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
[WIP] MobX 6 support #1569
Conversation
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. |
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. |
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 :(. |
Maybe the good folks at @infinitered will be interested in this since they are using mst on their ignite-cli. Tagging @jamonholmgren @rmevans9 |
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 If I can help with more testing, I will be happy to help. |
Can we create RC release on npm to simplify the testing a bit? |
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. |
I've released MST 4 & mst-middlewares 4 rc's on the @rc dist tag
I started working on it just before @mweststrate message. As an extension to what @mweststrate wrote, |
@mboperator would you mind testing your app with the I am more than happy to pair-programming and help you with the issues. |
Thanks for the RC, I'll test it out tomorrow! |
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", () => { |
There was a problem hiding this comment.
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
Apologies, I missed my tag on this. I’ll have my team test it! |
So far we've tried it on two projects and other than some IMO, we should merge it and release it, and then fix any edge cases in future patch-level releases. |
I'm willing to take on ownership of this repo and manage releases. My NPM username is |
@jamonholmgren done, thanks! |
My first act as Supreme Commander will be to rename it |
@jamonholmgren You bet we will ;-). |
@jamonholmgren as long as it supports Jamon 6.0 ! |
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 You are the MAAAAN! How do I buy you a beer?! And the Portland Trail Blazers are the best team in the NBA. |
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. |
Thanks @jamonholmgren ! |
No description provided.