Ensure harness doesn't force or eagerly computes target, stop using raw target elsewhere
#57526
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
typescript-bot
added
Author: Team
For Uncommitted Bug
Merged
jakebailey
commented
Feb 25, 2024
Comment on lines
260
to
262
| if (compilerOptions.newLine === undefined) compilerOptions.newLine = ts.NewLineKind.CarriageReturnLineFeed; | ||
| if (compilerOptions.skipDefaultLibCheck === undefined) compilerOptions.skipDefaultLibCheck = true; | ||
| if (compilerOptions.noErrorTruncation === undefined) compilerOptions.noErrorTruncation = true; |
There was a problem hiding this comment.
I want to say that we should drop these too, but:
- If we change the default
newLine, all baselines will change from CRLF to LF and lose git history. skipDefaultLibCheckis a perf optimization.noErrorTruncationgives us better baselines.
jakebailey
commented
Feb 25, 2024
| @@ -350,7 +351,7 @@ export function provideInlayHints(context: InlayHintsContext): InlayHint[] { | |||
| } | |||
|
|
|||
| function leadingCommentsContainsParameterName(node: Node, name: string) { | |||
| if (!isIdentifierText(name, compilerOptions.target, getLanguageVariant(file.scriptKind))) { | |||
| if (!isIdentifierText(name, getEmitScriptTarget(compilerOptions), getLanguageVariant(file.scriptKind))) { | |||
There was a problem hiding this comment.
It seems quite easy to make this kind of error; I am tempted to write an eslint rule which disallows the use of any CompilerOption property that we know to be computed, but that is non-trivial.
jakebailey
commented
Feb 25, 2024
| @@ -18,6 +18,136 @@ | |||
| "File '/lib/cjs/index.tsx' does not exist.", | |||
| "File '/lib/cjs/index.d.ts' exists - use it as a name resolution result.", | |||
| "======== Module name '../lib' was successfully resolved to '/lib/cjs/index.d.ts'. ========", | |||
| "======== Resolving module '@typescript/lib-esnext' from '/.src/__lib_node_modules_lookup_lib.esnext.d.ts__.ts'. ========", | |||
There was a problem hiding this comment.
I believe this baseline change to be correct; this test would (outside of the test harness) have an implicit target of esnext, but we were forcing it to be es5.
jakebailey
commented
Feb 25, 2024
| @@ -1,13 +1,8 @@ | |||
| error TS2468: Cannot find global value 'Promise'. | |||
| /src/bar.cts(2,1): error TS2712: A dynamic import call in ES5/ES3 requires the 'Promise' constructor. Make sure you have a declaration for the 'Promise' constructor or include 'ES2015' in your '--lib' option. | |||
There was a problem hiding this comment.
Another example of the implicit module setting now being reflected in tests; this was defaulted to ES5, but now it's esnext.
sheetalkamat
approved these changes
Feb 26, 2024
andrewbranch
approved these changes
Feb 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pulled out of #57525 to make that change less noisy.
targetin certain conditions; this had the affect of making some tests not test the implicit target controlled by themoduleoption.target; eagerly computing it had the affect of making our nice "this setting is dead, please remove" tests not work (because a change like Remove target=es3 #57525 would changetargetfrom ES3 into ES5 at use).targetdirectly, when they should have been using the computed target.