Implement 2022.3/Stage 3 Decorator Support #3638

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!

packages/mobx/src/api/action.ts

packages/mobx/src/types/actionannotation.ts

packages/mobx/src/types/computedannotation.ts

packages/mobx/src/types/observableannotation.ts

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. 👍

docs/enabling-decorators.md

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-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!

docs/enabling-decorators.md

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

package.json

        "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

packages/mobx/__tests__/decorators_20223/stage3-decorators.ts

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


packages/mobx/src/types/decorator_fills.ts

packages/mobx/src/types/flowannotation.ts

packages/mobx/src/types/observableannotation.ts

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

packages/mobx/src/types/observableannotation.ts

+            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

packages/mobx/src/types/observableannotation.ts

+        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

docs/enabling-decorators.md

+
+## 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 force-pushed the decorators2022 branch 7 times, most recently from 6f41744 to 5ce0269 Compare February 28, 2023 18:48

Matchlighter force-pushed the decorators2022 branch 6 times, most recently from 6de073c to 0908ca6 Compare March 12, 2023 05:13

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

Matchlighter force-pushed the decorators2022 branch from 0908ca6 to 50c5de9 Compare March 12, 2023 23:54

rickpastoor reviewed Mar 16, 2023

View reviewed changes

docs/enabling-decorators.md Outdated Show resolved Hide resolved

Matchlighter force-pushed the decorators2022 branch from 2fd5d0f to 224f162 Compare March 16, 2023 20:02

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

1 task

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.

Implement 2022.3/Stage 3 Decorator Support #3638

Implement 2022.3/Stage 3 Decorator Support #3638

Conversation

Matchlighter commented Feb 24, 2023 • edited Loading

Code change checklist

changeset-bot bot commented Feb 24, 2023 • edited Loading

🦋 Changeset detected

mweststrate left a comment

Choose a reason for hiding this comment

Matchlighter commented Feb 28, 2023

mweststrate commented Mar 1, 2023

Matchlighter commented Mar 1, 2023

Matchlighter commented Mar 12, 2023 • edited Loading

iChenLei commented Mar 12, 2023

Matchlighter commented Mar 12, 2023 • edited Loading

kubk commented Mar 13, 2023 • edited Loading

Matchlighter commented Mar 13, 2023

Amareis commented Mar 22, 2023

Matchlighter commented Mar 23, 2023

Amareis commented Mar 23, 2023 • edited Loading

Matchlighter commented May 26, 2023 • edited Loading

urugator commented May 27, 2023 • edited Loading

Matchlighter commented May 27, 2023 • edited Loading

derekcannon commented Jun 21, 2023

Matchlighter commented Jul 6, 2023

mweststrate commented Jul 9, 2023

mweststrate commented Jul 25, 2023

mweststrate left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matchlighter commented Jul 25, 2023

mweststrate left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mweststrate commented Aug 29, 2023

TomasChmelik commented Oct 11, 2023

mweststrate commented Oct 11, 2023 via email

mweststrate commented Oct 20, 2023 • edited Loading

mweststrate commented Nov 11, 2023

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 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

mweststrate commented Oct 20, 2023 •

edited

Loading