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

feat(51000) - Flag Deprecation Plan #51424

Merged
merged 16 commits into from
Dec 15, 2022
Merged

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #51000

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Nov 7, 2022
@a-tarasyuk
Copy link
Contributor Author

Many tests don't set target explicitly and rely on the ES3, therefore, the PR contains many changes related to tests

options.target = ts.getEmitScriptTarget(options);

export function getEmitScriptTarget(compilerOptions: {module?: CompilerOptions["module"], target?: CompilerOptions["target"]}) {
return compilerOptions.target ||
(compilerOptions.module === ModuleKind.Node16 && ScriptTarget.ES2022) ||
(compilerOptions.module === ModuleKind.NodeNext && ScriptTarget.ESNext) ||
ScriptTarget.ES3;
}

Is it acceptable to set globally ignoreDeprecations for tests?

@Jack-Works
Copy link
Contributor

Many tests don't set target explicitly and rely on the ES3, therefore, the PR contains many changes related to tests

can you create a new pr to explicitly set tests target to es5?

@a-tarasyuk
Copy link
Contributor Author

can you create a new pr to explicitly set tests target to es5?

I can, however many changes will remain due to the difference in output between es3 and es5.

@Jack-Works
Copy link
Contributor

can you create a new pr to explicitly set tests target to es5?

I can, however many changes will remain due to the difference in output between es3 and es5.

but that PR won't need to be reviewed clearly

@Jack-Works
Copy link
Contributor

I suggest to deprecate --experimentalDecorators and add a new flag --legacyDecorators to migrate

@RyanCavanaugh
Copy link
Member

Some notes from reviewing this:

  • Thanks for taking this on!
  • I'll send a PR shortly to change the default target to ES5. The test impact isn't too bad but it's big indeed.
  • In general, I think this code is "too maintanable" as it were. We'll need to update these things every 2.5 years or so, so simply hardcoding the disallowed values altogether should be simpler to read IMO. The "phases" as we have them don't need to be manifested in the code; when 5.5 comes around we'll (hopefully) simply be deleting values from enums. Hope that makes sense and can make this a bit shorter.

@a-tarasyuk
Copy link
Contributor Author

In general, I think this code is "too maintanable" as it were. We'll need to update these things every 2.5 years or so, so simply hardcoding the disallowed values altogether should be simpler to read IMO. The "phases" as we have them don't need to be manifested in the code; when 5.5 comes around we'll (hopefully) simply be deleting values from enums. Hope that makes sense and can make this a bit shorter.

I've removed the parse versions in favor of the hardcoded versions and left only the type required for CompilerOptions. Maybe you have ideas for further simplification...

"too maintainable"

😂 - this is the first time I've seen such a polite synonym for "these changes don't meet expectations".

@RyanCavanaugh
Copy link
Member

I think the above comment is the only one I have left - LGTM otherwise. Great work!

@a-tarasyuk a-tarasyuk marked this pull request as ready for review December 14, 2022 14:34
@typescript-bot
Copy link
Collaborator

The TypeScript team hasn't accepted the linked issue #51000. If you can get it accepted, this PR will have a better chance of being reviewed.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Thanks @a-tarasyuk!

@RyanCavanaugh RyanCavanaugh merged commit fe18527 into microsoft:main Dec 15, 2022
@RyanCavanaugh
Copy link
Member

🎉!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Flag Deprecation Plan
5 participants