-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
🚀 Proposal: MobX 6: 🧨drop decorators,😱unify ES5 and proxy implementations, 💪smaller bundle #2325
Comments
You probably already know this 😀 but here is my strong vote against removing decorators. I frankly don't care what TC39 think, decorators are here to stay. I'm not removing them from any codebase and I will strongly advocate against any removal from any library as well. IMO its time to stand our ground. |
Quick thought: |
I worry that this data/metadata duality will backfire eventually. If I remember correctly we had something like this in Mobx2.
|
Personally I also hate what TC39 is doing to decorators, and maybe it is one of those things that should be left to transpilers. Maybe there could be a mobx-decorators v7 package for those that want to keep using with them? (and also could make adoption easier).
Sounds awesome.
As long as there are alternatives it should be ok (mobx-keystone for example relies on both observer, intercept and then some more). But the more changes there are, the more likely current mobx libs will become incompatible and stop adoption.
When is a class considered simple? When there are no fields with mobx "decorators"?
I think 99% of the time you'd want functions to become actions (and actually I didn't even know this observable.ref was the case), so as long as this is explained on some release notes it should be ok. |
mobx-logger depends on |
@spion Frankly, that is just a rant. Michel wrote the pros of removing decorators. If you want to "stand ground" and "strongly advocate", then please make constructive counter-arguments instead of just "I like them". Personally, I am on the other side of this war and I never liked decorators. This fresh new look definitely makes sense to me. However, if there is some possibility to keep decorators support in the external package as it was suggested, then it's probably something we should do. If people feel the necessity to wear a foot gun, it's their choice. Besides, it would be suddenly more apparent where that decorator magic is coming from and that it's something optional. |
This comment has been minimized.
This comment has been minimized.
No, being serious here 🙈. Missed opportunity though!
Op do 2 apr. 2020 06:50 schreef Nir Weber <[email protected]>:
… April fools @mweststrate <https://github.com/mweststrate> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2325 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBGYSQOTJNWFY5ZPUYLRKQRTHANCNFSM4LZQ2B5Q>
.
|
Yeah, I think that is the part I like the least as well. Maybe we should introduce a separate 'marker' to for observable properties, e.g. |
I like decorators but I totally understand this direction. There are a few reasons for this, let me try and make Gorgi's case @FredyC :
Also, removing something so widely used because the spec committee can't proceed always leaves a bad taste in my mouth. Even as I acknowledge they are all acting in good faith and that decorators are a hard problem for engines because of the reasons outlined in the proposal (I've been following the trapping decorators discussions). |
Some API bikeshedding: Decorators class Doubler {
@observable value = 1
@computed get double() {
return this.field * 2
}
@action increment() {
this.value++
}
} Michel's original: class Doubler {
value = 1
get double() {
return this.field * 2
}
increment() {
this.value++
}
constructor() {
autoInitializeObservables(this)
}
} Subclass: class Doubler extends ObservableObject {
value = 1
get double() {
return this.field * 2
}
increment() {
this.value++
}
} Can be interesting, but is not very good in terms of coupling. Or with a class wrapper: const Doubler = wrapObservable(class Doubler {
value = 1
get double() {
return this.field * 2
}
increment() {
this.value++
}
}); |
Don't forget there will be a codemod to make most of the hard work for you, so this isn't really a valid argument. Besides, nobody is forced to upgrade to the next major. It probably depends if we will be able to maintain v4 & v5 or abandon those. And if we separate package will exist for decorators then it might be fairly non-braking change. Btw, @mweststrate Just realized, why MobX 7? Do we need to skip V6 for some reason? 🤓 |
Doh, I can't count.
Subclassing doesn't work, as the fields are not visible to the subclass
when it runs its constructor. Wrapping is probably a bit scary for many
people, but more importantly it is troublesome in TypeScript, as generic
arguments aren't nicely preserved that way, and with circular module
dependencies, as const expressions are not hoisted the same way as classes
IIRC (that is an recurring issue in MST).
…On Thu, Apr 2, 2020 at 10:56 AM Daniel K. ***@***.***> wrote:
- Decorators have been the "mostly standard MobX way" for a while and
removing them is 5 years of breakage.
Don't forget there will be a codemod to make most of the hard work for
you, so this isn't really a valid argument. Besides, nobody is forced to
upgrade to the next major. It probably depends if we will be able to
maintain v4 & v5 or abandon those. And if we separate package will exist
for decorators then it might be fairly non-braking change.
Btw, @mweststrate <https://github.com/mweststrate> Just realized, why
MobX 7? Do we need to skip V6 for some reason? 🤓
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2325 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBE6PWAMDR7QF4WVJFDRKROL7ANCNFSM4LZQ2B5Q>
.
|
@FredyC hey, I would prefer it if we avoided terms like "isn't really a valid argument" when talking about each other's points. I think having a common and standard way to do something in a library (decorators) is definitely a consideration and API breakage is a big concern - even with a codemod. I think removing decorators is unfortunately the way forward - but breaking so much code for so many users definitely pains me. @mweststrate subclassing is also not very ergonomic and mixes concerns here IMO. I'm not sure I understand the wrapping issue in TypeScript but I know there are challenges involving it. Wrapping doesn't actually have to change the type similarly to initializeObservables: class Doubler {
...
}
initializeObservables(Doubler); // vs in the constructor Or even decorate the type in a non-mutating way: const ReactiveDoubler = initializeObservables(Doubler); // vs in the constructor Wouldn't it make more sense to initializeObservables on the type and not on the instance? Is there any case I'd want to conditionally make an instance reactive but not have several types? |
@benjamingr yeah that is exactly what So even if you don't know the fields, but you do specify them on the type, you can't decorate the type to trap the assignment, because the new property will simply hide it. I think it is still a weird decisions to standardize |
@FredyC It is not a rant, it's a constructive comment. Decorators are widely used throughout the community, with large projects such as Angular, NestJS and MobX taking advantage of them. Thousands of projects depend on them. For TC39 to block their standardization process strikes me as extremely irresponsible, and the arguments for doing so are severely under-elaborated (a vague 3-pager does not an elaboration make - try harder TC39). The advantages that @mweststrate mentioned are largely the fault of this lackluster standardization which means the argument is cyclic: it ultimately comes down to "we're not supporting decorators because decorators are not well supported". Language features that aren't yet standardized are never well supported, the argument can be used to justify not adopting any new language feature. So if TC39 "paves cowpaths" and everyone adopted this way of thinking, the language would stop evolving. (Clearly, this is not a new feature so there is some merit to the "not likely to be supported" argument implying its a good idea to give up on them. I just wanted to bring to the table that the other approach - standing our ground - might be good too) For those of us who do care about decorators, what are our options? Our only hope is to stand our ground, keep using them and keep advocating their standardization. Even if TC39 doesn't standardize them, development within TypeScript might continue, addressing the remaining gaps WRT reflection and types. If you don't care about decorators, please stay out of it. They have always been optional in MobX and will continue to be optional - no one is forcing you to use them. If you care about a smaller bundle, they can be offloaded to a side module (but I maintain they should still have a first-class place in the documentation) |
@mweststrate is there anything stopping us from trapping construct and intercepting those fields then or setting a proxy? Such an That is:
That way the fact the field is not a part of the type doesn't really matter - since while it looks like we are decorating the prototype/class we are actually decorating the instance and only "decorating" the construct on the type. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Perhaps if the API were as follows, the construction performance could be improved at a slight cost in DX: class Foo {
@observable foo = 123
constructor() {
makeObservable(this, Foo)
}
}
class Bar extends Foo {
@observable bar = 456
constructor() {
super()
makeObservable(this, Bar)
}
}
class Baz extends Bar {
@observable baz = 'hello'
constructor() {
super()
makeObservable(this, Baz)
}
}
const baz = new Baz
autorun(() => {
console.log(baz.foo, baz.bar, baz.baz)
}) It would be faster because the decorators could map constructors to keys (f.e. |
@trusktr I think this is an interesting idea and one should not forget performance in the whole matter. |
I haven't made any comparisons, but if we have 1000 instances of the above Not sure if it is worth it, but perhaps having both is an option: |
Even in Mobx4/5 there is a logic similar to mobx/src/v5/utils/decorators.ts Line 43 in 9a4d67f
Moving the call to constructor actually makes it a lot less complicated. I don't say a one is necessarily faster than other, just saying it's not simply 3 vs 3000 calls. |
We have benchmark tests for creating 100.000 class instances. The results are as follow:
So things have became a bit slower indeed. I can imagine there are some optimisation opportunities here and there still. But the change isn't in a different order of magnitude, and so far object creations or updates being to slow have never been reported as an issue with mobx (IIRC), so I'm not too worried for now; it is quite likely you hit other bottlenecks first when dealing with 100k object, so without real life scenarios in which MobX object interactions prove to be the bottleneck, I'm not worried at this moment. I suspect however that the primary reason for the slow-down is the babel / TS config change to use |
Ouch - that link you shared really shows how bad this change regarding define semantics for class fields is. |
honestly I would worry too much about such microbenchmark, they give really
little relevant information. First of all, engines might still be catching
up with their optimizations and a few months from now things might look
entirely different. Secondly object creating and field assignments is
probably like 0.01% of the work your app is doing and unlikely to be the
bottleneck of your application, and not making a difference in user
experience if you make it either 3 times slower or faster.
…On Thu, Sep 10, 2020 at 11:52 PM Björn Zeutzheim ***@***.***> wrote:
Ouch - that link you shared really shows how bad this change regarding
define semantics for class fields is.
Would it be possible to run a v6 benchmark without the transpilation to
define semantics enabled so it can be exluded from the comparison?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2325 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBGY7HYADII4UYU77CLSFFKBZANCNFSM4LZQ2B5Q>
.
|
Edit: Fixed in [email protected] Not sure if this worth a separate issue... I decided to post it here as it's something new introduced with MobX 6: When upgrading from TypeScript 3.9.7 to TypeScript 4.0.2 I'm receiving an error when providing a second argument to Here's a code sandbox, relevant section highlighted: https://codesandbox.io/s/ecstatic-lake-s93v4?file=/src/App.tsx:351-480 The Code Sandbox is currently fine as it is using TypeScript version 3.9.7 which can be seen in the lower right corner. It seems to me that I cannot change the TS version inside CodeSandbox. However, when downloading that via Menu File→Export as Zip, doing
The same error is also marked in latest VS Code (August Update, which is using TS 4.0.2) but was not yet marked in the July Update (which is using TypeScript 3.9.7 like the Code Sandbox). |
I'm not sure about that - especially when having something like a virtual repeat which loads pages of entities, even small delays could cause the view to lag. |
Honestly... I am sad to see the decorators go. I am big fan of their simplicity and clarity. I am happy to see that there will be a work around to be able to continue to use them. However, I am thinking it will be quick to adapt to the new syntax I am guessing this choice was probably a good call. Maybe now without decorators I am hopeful I will be able to convey to people how amazing Mobx is and at least I won't hear the decorators excuse anymore. I have never seen an "@" sign scare so many people. |
@mweststrate if you have some time, could you give us an update on what is left to do before we can maybe get a mobx v6 beta and when we (roughly) might expect a v6 release? |
@olee -- I don't know about the release schedule but I'm using RC7 in production already. See also https://www.npmjs.com/package/mobx/v/6.0.0-rc.7?activeTab=versions. Except for my issue with TS 4.x described above, it works fine (with mobx-react-lite v.3 beta https://www.npmjs.com/package/mobx-react-lite/v/3.0.0-beta.1). However, you should be aware that there's no mobx-utils prerelease yet. See mobxjs/mobx-utils#276. |
@bb thanks for pointing that out, after upgrading MobX to 4.0.2 the internal tests fail as well |
New beta available: [email protected] I don't expect any API changes anymore, still polishing docs |
@mweststrate I confirm the TS 4.0.2 issue is fixed. Thanks a lot; both for fixing that and for releasing the mobx-utils RC. |
so @mweststrate, decorators are out in v6? It doesn't exists in the new docs anymore, but will using them fail? |
|
@mweststrate is there any concerns using MST with v6? |
I'd like to note that |
@yordis there is an open PR that should support MobX 6 fully. But I do not intend to release it personally as there are enough people and companies actually using the thing, benefiting from it but not really contributing back. So I leave releasing and double checking the update to the community, but the PR is here: mobxjs/mobx-state-tree#1569 |
@episage if I run it to terser's tree-shaking, I get results between 13.9 and 15.2 k depending on the imports ( |
@mweststrate I am sorry, I am confused with what you said.
Are you saying that people are using the package but they don't contribute back, therefore, you will no release a new version using
So we (the community) should take the lead on upgrading MST itself, and verify that works Is that correct? |
Exactly, I think all technical issues are addressed at this point. If
someone steps as volunteer to take care of doing the final steps and
releasing, and keep an eye on the project I'll gladly provide the necessary
permissions. If nobody is interesting in investing time, I don't see any
motivation / reason to put my time into it either :).
…On Mon, Sep 21, 2020 at 8:40 PM Yordis Prieto ***@***.***> wrote:
@mweststrate <https://github.com/mweststrate> I am sorry, I am confused
with what you said.
But I do not intend to release it personally as there are enough people
and companies actually using the thing, benefiting from it but not really
contributing back
Are you saying that people are using the package but they don't contribute
back, therefore, you will no release a new version using ***@***.***?
So I leave releasing and double checking the update to the community
So we (the community) should take the lead on upgrading MST itself, and
verify that works
Is that correct?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2325 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBDD6O77FQRAFAPWCHDSG6T4FANCNFSM4LZQ2B5Q>
.
|
Hey, I started to try mobx 6, and I have few questions and problems here is the commit with the new mobx setup
Thanks |
Closing since released: https://michel.codes/blogs/mobx6 For any follow up questions, open a separate issue. |
MobX 6
Hi folks, I've tinkered a lot about MobX 6 lately, so I want to layout the vision I have currently
Goals
🧨 1. Become compatible with modern ES standards
Let's start with the elephant in the room.
I think we have to drop the support for decorators.
Some have been advocating this for years, others totally love decorators.
Personally I hate to let decorators go. I think their DX and conciseness is still unparalleled.
Personally, I am still actively engaged with TC-39 to still make decorators happen, but we are kinda back to square one, and new proposal will deviate (again) from the implementation we already have.
Dropping decorators has a few advantages, in order of importance (imho)
[[define]]
over[[set]]
semantics, all our decorator implementations (and thedecorate
utility) are immediately incompatible with code that is compiled according the standard. (Something TypeScript doesn't do, yet, by default, as for TS this is a breaking change as well, unrelated to decorators). See #2288 for more backgrounddecorate
). I expect to drop a few KB by simply removing them.The good news is: Migrating a code base away from decorators is easy; the current test suite of MobX itself has been converted for 99% by a codemod, without changing any semantics (TODO: well, that has to be proven once the new API is finalized, but that is where I expect to end up). The codemod itself is pretty robust already!
P.s. a quick Twitter poll shows that 2/3 would love to see a decorator free MobX (400+ votes)
😱 2. Support proxy and non-proxy in the same version
I'd love to have MobX 6 ship with both Proxy based and ES5 (for backward compatibility) implementations. I'm not entirely sure why we didn't combine that anymore in the past, but I think it should be possible to support both cases in the same codebase. In Immer we've done that as well, and I'm very happy with that setup. By forcing to opt-in on backward compatibility, we make sure that we don't increase the bundle size for those that don't need it.
P.S. I still might find out why the above didn't work in the past in the near future :-P. But I'm positive, as our combined repo setup makes this easier than it was in the past, and I think it enables some cool features as well, such as detection of edge cases.
For example we can warn in dev mode that people try to dynamically add properties to an object, and tell them that such patterns won't work in ES5 if they have opted-in into ES5 support.
💪 3. Smaller bundle
By dropping decorators, and making sure that tree-shaking can optimize the MobX bundle, and mangling our source aggressively, I think we can achieve a big gain in bundle size. With Immer we were able to halve the bundle size, and I hope to achieve the same here.
To further decrease the build, I'd personally love to drop some features like
spy
,observe
,intercept
, etc. And probably a lot of our low-level hooks can be set up better as well, as proposed by @urugator.But I think that is a bridge too far as many already rely on these features (including Mobx-state-tree). Anyway I think it is good to avoid any further API changes beyond what is being changed in this proposal already. Which is more than enough for one major :). Beyond that, if goal 2) is achieved, it will be much easier to crank out new majors in the future :). That being said, If @urugator's proposal does fit nicely in the APIs proposed below, it might be a good idea to incorporate it.
4. 🛂Enable strict mode by default
The 'observed' one, that is.
🍿API changes
UPDATE 22-5-20: this issue so far reflected the old proposal where all fields are wrapped in instance values, that one hasn't become the solution for reasons explained in the comments below
This is a rough overview of the new api, details can be found in the branch.
To replace decorators, one will now need to 'decorate' in the constructor. Decorators can still be used, but they need to be opted into, and the documentation will default to the non-decorator version. Even when decorators are used, a constructor call to
makeObservable(this)
is needed, the type will be picked from the decoratorsmakeAutoObservable(this, exceptions?)
that will default to observable for fields, computed for getters, action for functionsProcess
Timeline
Whatever. Isolation makes it easier to set time apart. But from time to time also makes it less interesting to work on these things as more relevant things are happening in the world
CC: @FredyC @urugator @spion @Bnaya @xaviergonz
The text was updated successfully, but these errors were encountered: