-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Default target to "latest standard", deprecate ES5
#63067
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @mjbvz, and @joj for you. Feel free to loop in other consumers/maintainers if necessary. |
|
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. |
|
@typescript-bot test it |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
| computeValue: (compilerOptions): ModuleKind => { | ||
| return typeof compilerOptions.module === "number" ? | ||
| compilerOptions.module : | ||
| _computedOptions.target.computeValue(compilerOptions) >= ScriptTarget.ES2015 ? ModuleKind.ES2015 : ModuleKind.CommonJS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a later PR, I will update this to set other ModuleKinds based on the target.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
RyanCavanaugh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, I am OK with any proposed name for the "current" target version
d912f0e to
4f65b2b
Compare
target to "current year", deprecate ES5target to "latest standard", deprecate ES5
4f65b2b to
212085d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@typescript-bot run dt |
| ); | ||
| const reportDiagnostic = ts.createDiagnosticReporter(system); | ||
| const parsedConfig = ts.parseConfigFileWithSystem("tsconfig.json", {}, /*extendedConfigCache*/ undefined, /*watchOptionsToExtend*/ undefined, system, reportDiagnostic)!; | ||
| const parsedConfig = ts.parseConfigFileWithSystem("tsconfig.json", { target: ts.ScriptTarget.ES5, module: ts.ModuleKind.AMD }, /*extendedConfigCache*/ undefined, /*watchOptionsToExtend*/ undefined, system, reportDiagnostic)!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you're still using ES5 here - are you just doing this to avoid churning baselines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ryan asked me about this one too. Yes, this is to avoid churning baselines.
| compilerOptions: { | ||
| incremental: true, | ||
| target: "es5", | ||
| target: "es2015", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But so why is this one ES2015?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed most tests that broke to es2015 first, and then flipped things back. In this case I'm sure I did it to avoid a deprecation error that messes up the incremental test.
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
targetnow defaults toScriptTarget.LatestStandard, which is an alias to the most recent standardized ES version, currently ES2024 (until we add ES2025).target=ES5is now deprecated.#63066 got most test ready, but a few unit tests still needed to be updated. Individual commits may be a better way to review this.
Fixes #62196
Fixes #62198