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.

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

Fixes for decorators property deprecations #50343

Merged
merged 6 commits into from
Aug 19, 2022

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Aug 17, 2022

This fixes two issues that occur in TS 4.8 related to the deprecation of the decorators property on Node:

First, the deprecated decorators property used a type that made it difficult to find runtime breaks since use of decorators would silently succeed rather than fail to compile. To address this, I've changed the typeof decorators from NodeArray<Decorator> | undefined to undefined to make these breaks more obvious. As a result of this change, I renamed the decorators property to _decorators internally for cases where we track invalid grammar nodes (such as decorators parsed on a constructor, etc.), otherwise I would have run into a conflict with the definition of the types.

Second, the createConstructorDeclaration and updateConstructorDeclaration functions of NodeFactory incorrectly rejected a valid value for body when using the deprecated overloads due to an incorrect negation of a test.

Fixes #50259

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Aug 17, 2022
*/
readonly decorators: never;
/**
* @deprecated `modifiers` has been removed from `Node` and moved to the specific `Node` subtypes that support them.
Copy link
Member

Choose a reason for hiding this comment

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

are modifiers actually removed? or just more specific in their type on sub-nodes?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's intended that modifiers is not a property of every Node, but rather is a property of specific Node sub-types. i.e., you shouldn't just blindly read from node.modifiers.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good although I got confused by the wording of the last sentence of decorators' jsdoc.

src/deprecatedCompat/4.8/mergeDecoratorsAndModifiers.ts Outdated Show resolved Hide resolved
@rbuckton rbuckton force-pushed the deprecated-decorators-property branch from 146f69f to a2588b8 Compare August 19, 2022 17:18
@DanielRosenwasser
Copy link
Member

@typescript-bot cherry-pick this to release-4.8

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 19, 2022

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-4.8 on this PR at 46c30f3. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I've opened #50374 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Aug 19, 2022
Component commits:
da3e304 Change type of deprecated 'decorators' property

a63f47c fix 'Invalid Arguments' error for create/update constructor in factory

a2588b8 Update deprecation comments

aebe225 Make 'decorators' optional and 'undefined'

46c30f3 Rename '_decorators' to 'illegalDecorators'
@rbuckton rbuckton merged commit 284837d into main Aug 19, 2022
@rbuckton rbuckton deleted the deprecated-decorators-property branch August 19, 2022 18:27
@rbuckton
Copy link
Member Author

@DanielRosenwasser, we may need to re-create or update the cherry-pick PR.

@DanielRosenwasser
Copy link
Member

@typescript-bot cherry-pick this to release-4.8

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 19, 2022

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-4.8 on this PR at 6404716. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-4.8 manually.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Aug 19, 2022
Component commits:
da3e304 Change type of deprecated 'decorators' property

a63f47c fix 'Invalid Arguments' error for create/update constructor in factory

a2588b8 Update deprecation comments

aebe225 Make 'decorators' optional and 'undefined'

46c30f3 Rename '_decorators' to 'illegalDecorators'

6404716 Update baselines
DanielRosenwasser pushed a commit that referenced this pull request Aug 20, 2022
Component commits:
da3e304 Change type of deprecated 'decorators' property

a63f47c fix 'Invalid Arguments' error for create/update constructor in factory

a2588b8 Update deprecation comments

aebe225 Make 'decorators' optional and 'undefined'

46c30f3 Rename '_decorators' to 'illegalDecorators'

6404716 Update baselines

Co-authored-by: Ron Buckton <rbuckton@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecated decorator factory APIs throw "Invalid arguments" runtime error
4 participants