feat(51000) - Flag Deprecation Plan by a-tarasyuk · Pull Request #51424 · microsoft/TypeScript (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation17 Commits16 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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?
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?
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.
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
I suggest to deprecate --experimentalDecorators and add a new flag --legacyDecorators to migrate
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.
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".
I think the above comment is the only one I have left - LGTM otherwise. Great work!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
