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.

Sign up for GitHub

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

Jump to bottom

Update decorators to match latest spec #14353

Merged
merged 12 commits into from Mar 18, 2022
Merged

Update decorators to match latest spec #14353

merged 12 commits into from Mar 18, 2022

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Mar 13, 2022 •
edited by gitpod-io bot

Q                       A
Fixed Issues? Several decorators spec-compliance issues and updates
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT
  • Update initialize -> init
  • Update decorator application ordering:
    1. Static method decorators
    2. Proto method decorators
    3. Static field decorators
    4. Proto field decorators
  • Throw errors when addInitializer or metadata methods are called outside of decoration

Includes changes from #14339

@babel-bot
Copy link
Collaborator

babel-bot commented Mar 13, 2022 •
edited

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51511/

JLHwung
JLHwung reviewed Mar 13, 2022
View reviewed changes
...roposal-decorators/test/fixtures/2021-12-misc--to-es2015/leaked-context-get-metadata/exec.js Outdated
}

expect(() => {
getMetadata(metadataSymbol);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it throw if getMetadata is called in a static field decorator? If it throws then maybe we'd better move the test to a static field decorator so we are more confident that the context method is not invokable after the method decorators are applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should throw in the very next decorator, so we could apply a second decorator to the same element and have it call it

@nicolo-ribaudo nicolo-ribaudo added the PR: Spec Compliance 👓 A type of pull request used for our changelog categories label Mar 14, 2022
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Mar 14, 2022
Merged
@nicolo-ribaudo
Copy link
Member

Ignore the codecov failure, it will give an higher result when merged to main.

@pzuraq pzuraq force-pushed the update-decorators-to-match-latest-spec branch from cd8f7b7 to 1c57fcf Compare March 14, 2022 16:41
JLHwung
JLHwung reviewed Mar 14, 2022
View reviewed changes
packages/babel-helpers/src/helpers/applyDecs.js Outdated
newClass = nextNewClass;
}

decorationState.finished = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Should we also mark state as finished if applying decorator results to an abrupt completion?

I think we should, so the leaked context method will not be allowed to call again in a catch clause outside the class, but of course one can still use try-catch within the decorator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually think I need to update the spec to do this too, thanks for calling this out

pzuraq and others added 4 commits March 17, 2022 13:03
@pzuraq @nicolo-ribaudo
Update decorators to the latest spec
baef783 
- Update `initialize` -> `init`
- Update decorator application ordering:
  1. Static method decorators
  2. Proto method decorators
  3. Static field decorators
  4. Proto field decorators
- Throw errors when `addInitializer` or metadata methods are called
  outside of decoration
@nicolo-ribaudo
Smaller code (dedupe error message, managle prop name)
b00d11d 
5236 -> 5003 bytes (after terser)
@nicolo-ribaudo
Wrap decorationFinishedRef in try/finally
09bc0a1 
size: 5024 -> 5048
@nicolo-ribaudo
Dedupe code (5069 -> 5009 bytes after terser)
8c766c4
@nicolo-ribaudo nicolo-ribaudo force-pushed the update-decorators-to-match-latest-spec branch from 1c57fcf to b00d11d Compare March 17, 2022 12:38
@nicolo-ribaudo
Copy link
Member

I'd like to merge this soonish, since I'd like to release the fix for #14357 and the other decorators PR has already been merged.

I implemented all @JLHwung's suggestions, including #14353 (comment): even if it's a super edge case, it's what seems to be most coherent from a language design perspective.

JLHwung
JLHwung reviewed Mar 18, 2022
View reviewed changes
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Now that calling context.addInitializer after decoration is an error, do we still need

initializers = initializers.slice();

in pushInitializers? The leaked context method test was previously constructed for that.

JLHwung
JLHwung reviewed Mar 18, 2022
View reviewed changes
packages/babel-helpers/src/helpers/applyDecs.js
var decoratorFinishedRef = { v: false };

try {
var ctx = Object.assign(
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR: Object.assign is ES2015, we may need to import helpers.extends.

@nicolo-ribaudo
Copy link
Member

Oh good catch, .slice() is not needed anymore.

JLHwung
JLHwung reviewed Mar 18, 2022
View reviewed changes
packages/babel-helpers/src/helpers/applyDecs.js
initializers[i].call(instance);
}
return instance;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

The pushInitializer variant in applyClassDesc can be simplified, too.

@nicolo-ribaudo nicolo-ribaudo force-pushed the update-decorators-to-match-latest-spec branch from a91a39c to c2bf48f Compare March 18, 2022 18:33
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 18, 2022 •
edited

I deduped some more code, we are now at 4809 bytes after terser with vars managling enabled.

nicolo-ribaudo
nicolo-ribaudo approved these changes Mar 18, 2022
View reviewed changes
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

I made a bunch of minor helper refactors, but this is ready! I'll bump the helper version when releasing.

@nicolo-ribaudo nicolo-ribaudo merged commit 7316f93 into babel:main Mar 18, 2022
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 18, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees
No one assigned
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Decorators
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pzuraq @babel-bot @nicolo-ribaudo @JLHwung