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

Deprecate isParameter, rename to isParameterDeclaration #52283

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Deprecate isParameter, rename to isParameterDeclaration #52283

wants to merge 4 commits into from

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Jan 18, 2023 •
edited
Loading

This is a follow-up to #49929, which:

  • Renames isParameterDeclaration to isPartOfParameterDeclaration. Done in another PR
  • Renames isParameter to isParameterDeclaration (as that is the node name).
  • Adds a deprecated compat function isParameter.
  • Renames SyntaxKind.Parameter to SyntaxKind.ParameterDeclaration and adds a deprecated duplicate (like we did with JSDoc).

I made up the timing for this deprecation; I just copied the same scheme as another helper's but I feel like this one needs a loooot of time for people to fix.

This may not be the safest thing because isParameterDeclaration was previously a different function, so people may use the new one while still using old TS versions. Not great.
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 18, 2023
jakebailey
jakebailey commented Jan 18, 2023
View reviewed changes
src/deprecatedCompat/5.0/isParameter.ts Outdated
Comment on lines 10 to 14
// - soft: 5.0
// - warn: 5.1
// - error: TBD
Copy link
Member Author

Choose a reason for hiding this comment

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

These values are made up and are up for discussion.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah....SyntaxKind.Parameter is used quite a bit right now. We should probably have a very long warn period.
Copy link
Member Author

Choose a reason for hiding this comment

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

This comment is of course out of date, this missed 5.0 which was a shame.

I'm not totally sure what the plan here is anymore. I will note that we actually can't "remove" an enum item; there's not really a deprecation helper mechanism for that. So most people would work forever, it's just those who are using isParameter that will have a problem.
src/compiler/utilities.ts
@@ -4544,9 +4544,9 @@ export function isPushOrUnshiftIdentifier(node: Identifier) {
*
* @internal
*/
export function isParameterDeclaration(node: Declaration): boolean {
export function isPartOfParameterDeclaration(node: Declaration): boolean {
Copy link
Member Author

Choose a reason for hiding this comment

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

I personally preferred rootDeclarationIsParameter or similar, but this was the suggestion so I just chose it.
src/compiler/types.ts
Comment on lines +226 to +235
ParameterDeclaration,
/** @deprecated Use SyntaxKind.ParameterDeclaration */
Parameter = ParameterDeclaration,
Copy link
Member Author

Choose a reason for hiding this comment

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

This models what we did for JSDoc.
@jakebailey jakebailey mentioned this pull request Jan 18, 2023
Merged
@sandersn sandersn added this to Not started in PR Backlog Feb 1, 2023
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Feb 1, 2023
@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.
@jakebailey jakebailey force-pushed the is-parameter-declaration branch 3 times, most recently from 4f7a3e1 to 7d99242 Compare August 30, 2023 22:45
@jakebailey jakebailey mentioned this pull request Dec 11, 2023
Merged
@jakebailey jakebailey force-pushed the is-parameter-declaration branch 2 times, most recently from 6071d41 to d5f4d4b Compare January 22, 2024 20:34
@jakebailey jakebailey force-pushed the is-parameter-declaration branch 2 times, most recently from 1f86294 to 27426be Compare March 19, 2024 20:31
@jakebailey jakebailey mentioned this pull request Apr 4, 2024
Merged
@jakebailey jakebailey force-pushed the is-parameter-declaration branch 2 times, most recently from 3c14ca7 to e6c9ffc Compare April 19, 2024 15:29
@jakebailey jakebailey marked this pull request as draft April 19, 2024 15:32
@jakebailey
Copy link
Member Author

I'm going to pull the isParameterDeclaration -> isPartOfParameterDeclaration rename into its own PR; it's confused enough people working on TS, but thankfully nobody externally is using it: https://github.com/search?q=%2F%5Cbts%5C.isParameterDeclaration%5Cb%2F+%28language%3ATypeScript+OR+language%3AJavaScript%29&type=code
@jakebailey jakebailey mentioned this pull request Apr 19, 2024
Merged
@jakebailey jakebailey changed the title Rename isParameterDeclaration, deprecate isParameter Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
3 participants
@jakebailey @typescript-bot @DanielRosenwasser