Implement 2022.3/Stage 3 Decorator Support by Matchlighter · Pull Request #3638 · mobxjs/mobx

Matchlighter · 2023-02-24T22:08:38Z

Title kind of speaks for itself on this one.

Prior conversation has taken place in a discussion here: #3373 (comment)

Code change checklist

Added/updated unit tests
Updated /docs. For new functionality, at least API.md should be updated
Verified that there is no significant performance drop (yarn mobx test:performance)
Finish writing unit tests
Cleanup

changeset-bot · 2023-02-24T22:08:41Z

🦋 Changeset detected

Latest commit: 78bee62

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
mobx	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

mweststrate

Thanks @Matchlighter for picking this up! Looking quite sane so far, left a bunch of questions!

Matchlighter · 2023-02-28T18:51:46Z

Just pushed another patchset. All tests are passing locally (except eslint ones, which I haven't touched and are also giving me trouble when I run them against main branch). Everything should be pretty much done except some of the design decisions around enumerability.

mweststrate · 2023-03-01T08:54:34Z

Thanks for the awesome work so far @Matchlighter! The coming two weeks I'm too busy to give this PR the proper attention to wrap it up, push it forward etc, but I think this is strongly heading in the right direction, so feel free to already update docs etc. My plan is to first unblock @urugator on the React 18 work half March (#3590) and then I'll focus on decorators. (I'm feeling quite the bottleneck now😅)

Matchlighter · 2023-03-01T17:40:31Z

@mweststrate All good! I'm getting a bit busy too so it works well! I'll go ahead and work on getting things cleaned up and updating docs over then next week or so.

Matchlighter · 2023-03-12T05:17:15Z

Pushed a new change with updated doc and with local builds/tests completing successfully. It appears there's a regression in the type-checker's module resolution logic in the TypeScript 5.0 Beta and RC, but it's corrected in the nightly, so I bumped the TS resolution up to the most recent (as of writing this) nightly. There's also a (non-critical) TODO regarding eslint - it warns about being incompatible with TS 5, but otherwise works - will need to bump the typescript-eslint package(s) once it accepts TS 5 w/o warning to reduce log-spam when running tests.

iChenLei · 2023-03-12T06:25:37Z

Run yarn --frozen-lockfile --ignore-scripts
yarn install v1.22.19
[1/4] Resolving packages...
warning Resolution field "typescript@5.1.0-dev.20230311" is incompatible with requested version "typescript@^3.7.3"
warning Lockfile has incorrect entry for "typescript@^5.0.0-beta". Ignoring it.
warning Resolution field "typescript@5.1.0-dev.20230311" is incompatible with requested version "typescript@^5.0.0-beta"
error Your lockfile needs to be updated, but yarn was run with --frozen-lockfile.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
Error: Process completed with exit code 1.

Matchlighter · 2023-03-12T23:57:05Z

Wuh...? Latest yarn.lock is checked-in - running yarn doesn't change the yarn.lock file. Anyway, here's a tweak that looks like it helps that

Hrm. Seeing an issue with something about getMutableClone only when run by CI. Will look into tomorrow.

kubk · 2023-03-13T04:38:45Z

@Matchlighter Thank you so much for the PR. Just for you to know, you don't have to squash and force push all the time, cause we merge PRs with "squash" flag enabled. It seems like because of force push GitHub always requires us to approve and run your PR while usually it's enough to approve only once. I'll approve again.

Matchlighter · 2023-03-13T04:58:09Z

Ahh, gotcha. 👍

Amareis · 2023-03-22T10:04:04Z

@Matchlighter you can bump TS to 5.0.2, it's released now.

Matchlighter · 2023-03-23T04:28:31Z

@Amareis I've updated things, but I've discovered what seems to be a bug in TS 5.0.2 that wasn't present in the betas. I've submitted microsoft/TypeScript#53448 that was caught by one of MobX's specs. I've worked around it in the spec for now.

Amareis · 2023-03-23T06:49:00Z

One question about field actions (@action field = () => {}) - why does this require makeObservable? You can just return (mthd) => _createAction(mthd) and everything will work as intended. I use this syntax in some places and it will silently break after migration, which is not good IMHO.

Matchlighter · 2023-03-23T20:33:37Z

One question about field actions (@action field = () => {}) - why does this require makeObservable? You can just return (mthd) => _createAction(mthd) and everything will work as intended. I use this syntax in some places and it will silently break after migration, which is not good IMHO.

Talking with @mweststrate (#3638 (comment)), we both seemed to consider such syntax as unsupported with Stage 3 Decs since it has some other gotchas - eg re-assigning to such a field will not make the newly assigned function an action (stage3 decorators don't allow field decorators to define a setter) - a more semantic approach would be to use field = action(() => { ... }). We could make it work via the new accessor keyword, but re-assignable actions seem like a niche case and would still have some gotchas (if I set @action field to f(y), and then I read field, action(f(y)) is returned when one might be expected f(y) - there are side-effects that feel less than clean).

urugator · 2023-03-23T21:12:41Z

I did not follow the discussion or the code and I am not familiar with spec, feel free to ignore if it's completely off topic, just wanted to mention that explicit actions are not part of state and therefore should be non-writable. Everything writable is considered part of state and therefore must be observable.

mobx/packages/mobx/src/types/actionannotation.ts

Lines 101 to 103 in 8bea8cd

    
           // Non-obsevable, therefore non-writable 
        
           // Also prevents rewriting in subclass constructor 
        
           writable: safeDescriptors ? false : true

This is to avoid users wondering why certain mutations are reactive and some aren't - by default it's either mutable+observable or immutable+non-observable.
Also note that any function assigned to observable becomes autoAction (eg @observable fn = () => {}). Again this is to make sure everything runs in batch by default.

EDIT: Point being: @action field = () => {} is preferred over field = action(() => {}), because the former makes sure nobody can treat the field as a state. Actually the best version is @action.bound, because it can also be overriden by sublass.

Matchlighter · 2023-03-24T00:37:20Z

Thanks for the background @urugator. That makes sense regarding the state/non-state. I also agree that @action.bound is the ideal.

Given that @action.bound is available and @action field = () => { } (with makeObservable()) is not writable, are there any cases that it should be preferred over @action method() { }/@action.bound method() { }? If not, I don't see a point in increasing confusion and breaking some ES best-practices in order to add support for @action field = () => {} in the Stage 3 Decorator implementation.

I'd be more open if Stage 3 decorators allowed writable: false'ing a field, or if this was a breaking change (to me, the breaking part would be the app-dev removing makeObservable() without properly upgrading the class), but w/o either being the case, it seems like adding support for @action field = w/o makeObservable() would just cause more trouble than it'd fix.

Amareis · 2023-03-24T08:20:58Z

Single reason to prefer method over field is one subtle difference in typescript - object with method has signature {method(): void} and with function field {method: () => void}. This actually make a sense when you use eslint rule to preventing unbound method usage.
My main concern is just about unsound bug when v4/5 decorators (where action field is legit) will be run on v6 and such fields will be accepted by TS, but they will not be actually actions without makeObservable. I think this should be fully working as intended, or fully broken at least on type level.
Such cases can be transfomed by codemod, but still it will be emerging regularly when, for example, method will be converted to to field function via IDE actions.

Amareis · 2023-03-24T08:27:52Z

So, maybe it worth to support this behavior until legacy decorators support will be fully dropped? Naturally, it will be breaking change and dropping action support for fields will fit perfectly with it.

Matchlighter · 2023-03-24T17:04:40Z

I'm more considering the MobX 6 to MobX 6 w/ 20223 Decorators transition - are you more considering the case of transitioning directly from MobX 4/5 to MobX 6 w/ 20223 Decorators?

I can't make the field readonly as has been in the past (it's a limitation of 20223 field decorators), so it feels like I'd just be trading one issue (an eslint warning) for another (re-assignable actions that don't stay actions). Switching to Stage 3 decorators is already an opt-in, code-change-requiring feature, and it is especially so if transitioning directly from MobX 4/5 to Stage 3 Decorators.

I've tried to limit the impact when transitioning from MobX 6 by continuing to support field decorators when makeObservable() is also called, but wasn't able to do as much for the 4/5 to 20223 transition. Here's a TS Playground that shows an attempt, but my defineProperty is run before the TS-generated one and is thus replaced and ineffectual.

Amareis · 2023-03-26T05:05:20Z

Honestly, I don't think field actions is actually reassigned often, and even if so - does it actually differs from method reassigning? MobX cannot automatically wraps reassigned decorated methods in actions too, so mental model is same - action decorator applies only on initial function, and on reassigning you need to wrap new function manually. I think, it's much better documentation note than "don't forget to use makeObservable with field actions".

Also, if you really want to enforce this rule "only actions could be assigned to actions fields/methods", it can be made by adding brand protection on action, which is returned from decorator - so user can't just assign pure function to it. But it feels like overkill.

Amareis · 2023-03-26T05:11:14Z

And about transition from MobX 5 - it feels like you only need to add accessor keyword to any @observable field (if action fields without makeObservable will be a thing). So, it maybe even simpler than transition from makeObservable, which you need to delete from every constructor (if there is no action fields, currently).

mweststrate · 2023-03-31T19:08:55Z

Sorry for being delayed on this, but I'll try to do a thorough review of the PR again next week, and get back to the questions / ideas above!

mweststrate · 2023-04-07T20:00:54Z

Sorry again delaying reviewing / merging this PR a bit, until we have the open React issue solved, to avoid moving two pieces at the same time.

Amareis · 2023-04-26T10:40:13Z

Sorry again delaying reviewing / merging this PR a bit, until we have the open React issue solved, to avoid moving two pieces at the same time.

@mweststrate
Which exactly open React issue we are waiting for?

dartess · 2023-05-04T08:18:46Z

+## Usage
+
+```javascript
+import { makeObservable, observable, computed, action } from "mobx"


makeObservable is not used in this example.

at first I thought it should be removed from the example.

but then I saw this:

@action some_field = () => {} was and is valid usage (if makeObservable() is also used).

The example uses @action, but with a class method, not a class property.

What does it all mean? That the call to makeObservable is missing in the example? Or that makeObservable should only be used with class property methods? Should the documentation be clearer and not raise such questions?

makeObservable() is unneeded in this example. I'll fix when mwestrate has the opportunity to review again.

dartess · 2023-05-04T08:19:15Z

+
+    @action
+    toggle() {
+        this.finished = !finished


in this and other examples - !this.finished

Nice catch. I'll fix when I address whatever comments mwestrate makes.

Matchlighter · 2023-05-26T05:37:46Z

@kubk Is that CI error a known issue? It doesn't really seem to be related and I'm unable to repro locally.

urugator · 2023-05-27T09:58:30Z

@Matchlighter This test is unfortunately timing sensitive, it waits for GC. Was trying to find some reasonable value for the timeout, but it still occasionally fails depending on CI workload. Feel free to change it to 5000ms or whatever passes.

Matchlighter · 2023-05-27T16:17:51Z

@urugator Ah gotcha. I'll try playing with it. I know there's a hook in Node to force a GC - I'll try that and then try timing.

Edit: Ah you're already using that. Seems weird that it's not immediate - the project I used it in, it was reliably done by the next run of the event loop.

derekcannon · 2023-06-21T10:03:59Z

If makeObservable will not be required for the new decorators, does that mean we can mix annotation super classes with decorated child classes? Currently, there's an exception thrown about using makeObservable with the second argument when using decorators in a child class.

Matchlighter · 2023-07-06T20:15:13Z

@mweststrate Do you have an updated timeline of when you can look at this?

mweststrate · 2023-07-09T20:00:03Z

@Matchlighter sorry, I've been terrible swamped lately. But I expect we can release #3673 in the coming days, then I'll pick this one up first thing. Again apologies for being the bottleneck and the delay in reviewing your awesome work!

mweststrate · 2023-07-25T17:32:29Z

@Matchlighter Sorry, the React work hit more bumps than we hoped. The React work is not entirely wrapped up, but I think the remaining changes are local enough to be able to safely rebase this branch. Are you able to look into that or do you want me to take care of that?

mweststrate

Again, such an awesome job you did here @Matchlighter! I left a bunch of comments, but overall things look really good. I still want to give it a manual test drive, but beyond that I don't think much is in the way of releasing.

I'll send you a maintainers invite as well, that makes it a bit easier to contribute in the future as you can directly write to the repo. Feel free to reject or accept!

Also, again super sorry this is taking so long. You've put a lot of effort in here, it didn't deserve to be on the shelve for so long. Also, note that we can sponsor significant contributions, let me know via DM / mail if you want to make use of that!

mweststrate · 2023-07-25T18:41:51Z

+        "ts-jest": "^29.0.5",
        "tsdx": "^0.14.1",
-        "typescript": "^4.0.2"
+        "typescript": "^5.0.2"


If it is little hastle, it could be interesting to split out the tooling & infra updates (Jest and TS) from the actual changes as separate PR, which we can land quickly, given that space wise the largest updates is coming from Jest, it might be nice to have that hitched down before landing decorators, to be able to more easily roll back if needed.

Oof. That would have been a good thought for me to think of... I'll see what I can do.

mweststrate · 2023-07-25T18:57:02Z

+    })
+})
+
+test("verify object assign (2022.3) (legacy/field decorator)", () => {


mweststrate · 2023-07-25T19:22:27Z

+            false
+        )
+        adm.values_.set(name, observable)
+        initializedObjects.add(target)


asObservableObject is idempotent, so I'm not 100% we need the initializedObjects? This feels a bit irky, but probably that can possibly be optimised later, fine for now I guess. I imagine we could do a check with asObservableObject(target)[$mobx].values_.has if we still need to init.

That works too. This approach mimics how TS handles some of the accessor stuff in the transpiled code.

Reviving this discussion, it seems this might still change in the spec: tc39/proposal-decorators#513

I'm crossing my fingers! That changing would make this so much cleaner (and probably a little faster since we'll avoid the has() call

Matchlighter · 2023-07-25T19:42:39Z

Thanks for the review - I'll take a look in the next few hours. Looks like you already rebased 👍 .

mweststrate

Preliminary approval. Still want to do some manual testing and land the React fixes first.

Also cc @urugator love your thoughts on this if interested :) (if you can't manage no worries!)

phiresky · 2023-08-29T17:28:26Z

+        addInitializer(function () {
+            storeAnnotation(this, name, ann)
+        })
+        return


it would be great if there was a config option to forbid this or at least warn. seems to me that it will just be confusing when you have a code base and forget the accessor modifier and it just silently breaks

phiresky · 2023-08-29T17:37:53Z

+
+## MobX Core decorators {🚀}
+
+MobX before version 6 encouraged the use of ES.next decorators to mark things as `observable`, `computed` and `action`. While MobX 6 recommends against using these decorators (and instead using [`makeObservable` / `makeAutoObservable`](observable-state.md)), it is still possible.


This seems a bit confusing. Are they recommended for use now or not? Maybe this section should be titled "legacy decorators" and the text changed?

Good call! I'll went to the docs and there is still some more straightening out to do, but we're close!

mweststrate · 2023-08-29T18:23:06Z

The whole mobx-react update took a bit longer than planned 😅, but it has been released, so if it is stable for ~week we can continue with this one finally! I noticed it requires some more doc finetuning, I will follow up on that. Otherwise things look complete :)

TomasChmelik · 2023-10-11T10:16:36Z

Any news on this? I can't wait for the decorator support for MobX ❤️

mweststrate · 2023-10-11T21:31:57Z

Hey! Yeah it is primarily waiting on me to update documentation further. But I could make a pre release! It is code complete, but didn't want to release before the React 18 integration stabilised, which happened.

…

On Wed, 11 Oct 2023, 12:16 TomasChmelik, ***@***.***> wrote: Any news on this? I can't wait for the decorator support for MobX ❤️ — Reply to this email directly, view it on GitHub <#3638 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBE4QNDSWW6QIRPWJODX6ZWY7ANCNFSM6AAAAAAVHMVXNA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mweststrate · 2023-10-20T10:13:41Z

mweststrate · 2023-11-11T15:43:12Z

Branch has been merged through #3790, will announce future on Monday!

mweststrate reviewed Feb 25, 2023

View reviewed changes

Matchlighter marked this pull request as ready for review March 12, 2023 05:17

rickpastoor reviewed Mar 16, 2023

View reviewed changes

Comment thread docs/enabling-decorators.md Outdated

dartess reviewed May 4, 2023

View reviewed changes

Matchlighter added 2 commits May 25, 2023 21:30

add support for Stage3/2022.3 decorators

b7b1af8

bump Jest stack

23897ff

Matchlighter added 2 commits May 29, 2023 12:22

bump test GC timeout

63f42a7

try fix GC test

8015f89

derekcannon mentioned this pull request Jun 20, 2023

makeObservable throws in dev environment when used with annotations on decorated object #3225

Closed

mweststrate requested changes Jul 25, 2023

View reviewed changes

Merge branch 'main' into decorators2022

c5ff4f9

mweststrate requested a review from urugator July 25, 2023 19:33

minor cleanups

63acafc

mweststrate approved these changes Jul 26, 2023

View reviewed changes

phiresky reviewed Aug 29, 2023

View reviewed changes

Merge branch 'main' into decorators2022

78bee62

alexander-schranz mentioned this pull request Oct 30, 2023

Outdated javascript dependencies sulu/sulu#6343

Open

4 tasks

This was referenced Nov 1, 2023

Add computed.shallow annotation #2986

Open

[Local fork] TC 39 decorators #3790

Merged

mweststrate closed this Nov 11, 2023

github-actions Bot mentioned this pull request Nov 11, 2023

Next release #3793

Merged

urugator mentioned this pull request May 22, 2024

@action decorators on fields do not convert the field value into action #3879

Closed


		## MobX Core decorators {🚀}

		MobX before version 6 encouraged the use of ES.next decorators to mark things as `observable`, `computed` and `action`. While MobX 6 recommends against using these decorators (and instead using [`makeObservable` / `makeAutoObservable`](observable-state.md)), it is still possible.

Uh oh!

Uh oh!

Conversation

Matchlighter commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code change checklist

Uh oh!

changeset-bot Bot commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

mweststrate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Matchlighter commented Feb 28, 2023

Uh oh!

mweststrate commented Mar 1, 2023

Uh oh!

Matchlighter commented Mar 1, 2023

Uh oh!

Matchlighter commented Mar 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iChenLei commented Mar 12, 2023

Uh oh!

Matchlighter commented Mar 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kubk commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matchlighter commented Mar 13, 2023

Uh oh!

Uh oh!

Amareis commented Mar 22, 2023

Uh oh!

Matchlighter commented Mar 23, 2023

Uh oh!

Amareis commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matchlighter commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

urugator commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matchlighter commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amareis commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amareis commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matchlighter commented Mar 24, 2023

Uh oh!

Amareis commented Mar 26, 2023

Uh oh!

Amareis commented Mar 26, 2023

Uh oh!

mweststrate commented Mar 31, 2023

Uh oh!

mweststrate commented Apr 7, 2023

Uh oh!

Amareis commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Matchlighter commented Feb 24, 2023 •

edited

Loading

changeset-bot Bot commented Feb 24, 2023 •

edited

Loading

Matchlighter commented Mar 12, 2023 •

edited

Loading

Matchlighter commented Mar 12, 2023 •

edited

Loading

kubk commented Mar 13, 2023 •

edited

Loading

Amareis commented Mar 23, 2023 •

edited

Loading

Matchlighter commented Mar 23, 2023 •

edited

Loading

urugator commented Mar 23, 2023 •

edited

Loading

Matchlighter commented Mar 24, 2023 •

edited

Loading

Amareis commented Mar 24, 2023 •

edited

Loading

Amareis commented Mar 24, 2023 •

edited

Loading

Amareis commented Apr 26, 2023 •

edited

Loading

Matchlighter commented May 26, 2023 •

edited

Loading

urugator commented May 27, 2023 •

edited

Loading

Matchlighter commented May 27, 2023 •

edited

Loading

mweststrate left a comment •

edited

Loading

mweststrate left a comment •

edited

Loading