packageJsonWatch, testing package.json and impliedNodeFormat use in transformer #48958
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Changes to be committed: modified: src/testRunner/unittests/tsbuildWatch/publicApiImpliedNodeFormat.ts new file: tests/baselines/reference/tsbuildWatch/publicApi/with-custom-transformers,-test-that-impliedNodeFormat-is-set.js * tests the impliedNodeFormat is correctly set for custom transformer, in a simple case with compilerOptions:nodeResolution set to Node12
Changes to be committed: modified: src/testRunner/unittests/tsbuildWatch/publicApiImpliedNodeFormat.ts modified: tests/baselines/reference/tsbuildWatch/publicApi/with-custom-transformers,-test-that-impliedNodeFormat-is-set.js * add test to change package.json type from coomonjs tp module => impliedNodeFormat correctly changes * add test to change package.json data only => project rebuild (not really desired, but it doesn't affect correctness)
Changes to be committed: modified: src/testRunner/unittests/tsbuildWatch/publicApiImpliedNodeFormat.ts modified: tests/baselines/reference/tsbuildWatch/publicApi/with-custom-transformers,-test-that-impliedNodeFormat-is-set.js * adding tests for other combinations of module and moduleResolution | module | moduleResolution | uses package.json:type | |-- |-- |-- | | unset | "node12" | true | | "node12" | unset | true | | "node12" | "node" | false |
There was a problem hiding this comment.
I don't understand the purpose of this PR. @sheetalkamat or @andrewbranch ?
| builder.build(); | ||
| runWatchBaseline({ | ||
| scenario: "publicApi", | ||
| subScenario: "with custom transformers, test that impliedNodeFormat is set", |
There was a problem hiding this comment.
I don't know how good filesystems are at handling , -- I feel like the escaping code in runWatchBaseline should remove it.
There was a problem hiding this comment.
Yes it comes out ugly with-custom-transformers,-test-that-impliedNodeFormat-is-set. I'll normalize it.,
There was a problem hiding this comment.
Changed to
subScenario: "with custom transformers impliedNodeFormat",
in now uploaded code.
| }); | ||
| } | ||
| // function printModuleResolutionCache(buildr: SolutionBuilder<EmitAndSemanticDiagnosticsBuilderProgram>){ | ||
| // const x = { moduleResolutionCache: buildr.getModuleResolutionCache() }; |
There was a problem hiding this comment.
what is this test testing? I expected to see getModuleResolutionCache somewhere, but it's commented out here
There was a problem hiding this comment.
This test is writing and changing package.json "type" fields (values "commonjs" or "module"), and checking that the change detected is propagated up to the SourceFile["impliedNodeFormat"] field - in the context of a SolutionBuilder watch build.
The getModuleResolutionCache is apparently not used at all (and will not be used). I didn't know that, and put it there as part of a question I asked @sheetalkamat. I will remove it.
There was a problem hiding this comment.
The commented out code referring to getModuleResolutionCache is removed in the now uploaded code.
Changes to be committed: modified: src/compiler/tsbuildPublic.ts The public accessor for getModuleResolutionCache is removed modified: src/testRunner/unittests/tsbuildWatch/publicApiImpliedNodeFormat.ts Baseline description simplified, resulting in more legible naming of the baseline renamed: tests/baselines/reference/tsbuildWatch/publicApi/with-custom-transformers,-test-that-impliedNodeFormat-is-set.js -> tests/baselines/reference/tsbuildWatch/publicApi/with-custom-transformers-impliedNodeFormat.js
|
Unfortunately, we haven't had time to review this PR and there's no issue showing an intent to add it to Typescript. I think it makes most sense to close this PR and re-open it if something changes. |
github-project-automation
bot
moved this from Waiting on reviewers
to Done
in PR Backlog
With regards to "Handle package.json watch in tsc, tsc --build and tsserver" #48889
@sheetalkamat
@weswigham
@andrewbranch
(C.f. the now closed question #48794 where @andrewbranch and @weswigham explained how to use
SourceFile.impliedNodeFormat.)I saw this pull and assumed the content is necessary to get correct values for
SourceFile.impliedNodeFormat.Therefore on the
packageJsonWatchbranch I created a test to check the value of impliedNodeFormat from inside a transformer whenFrom the transformer, the value of
impliedNodeFormatis correctlyModuleKind.CommonJS