Make `module` control `impliedNodeFormat` and `moduleResolution` control just module resolution #54788

andrewbranch · 2023-06-26T20:02:17Z

Follow-up to #54567. This helps establish a mental model that's easier to document, and gives us more room to fix stuff like #54102.

…eFormat

tests/baselines/reference/tsc/moduleResolution/node10Result.js

src/testRunner/unittests/tscWatch/forceConsistentCasingInFileNames.ts

DanielRosenwasser · 2023-06-26T20:43:03Z

@typescript-bot pack this
@typescript-bot test this
@typescript-bot test top240
@typescript-bot user test this
@typescript-bot user test tsserver
@typescript-bot test tsserver top160
@typescript-bot run dt

typescript-bot · 2023-06-26T20:43:06Z

Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 081a381. You can monitor the build here.

typescript-bot · 2023-06-26T20:43:06Z

Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at 081a381. You can monitor the build here.

typescript-bot · 2023-06-26T20:43:06Z

Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at 081a381. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-06-26T20:43:06Z

Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 081a381. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-06-26T20:43:06Z

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 081a381. You can monitor the build here.

typescript-bot · 2023-06-26T20:43:06Z

Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at 081a381. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-06-26T20:43:06Z

Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at 081a381. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-06-26T20:44:59Z

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/155658/artifacts?artifactName=tgz&fileId=CB74F1F1E3C0805CB6B63096DD69ECEEA20A41124B0619354C7EF4C28AAF066102&fileName=/typescript-5.2.0-insiders.20230626.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

src/testRunner/unittests/helpers/node10Result.ts

typescript-bot · 2023-06-26T21:06:57Z

@DanielRosenwasser Here are the results of running the user test suite comparing main and refs/pull/54788/merge:

Everything looks good!

typescript-bot · 2023-06-26T21:12:43Z

@DanielRosenwasser Here are the results of running the user test suite comparing main and refs/pull/54788/merge:

There were infrastructure failures potentially unrelated to your change:

1 instance of "Unknown failure"
1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

rxjs-src

/mnt/ts_downloads/rxjs-src/build.sh

[NEW] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.collection.d.ts(63,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.iterable.d.ts(162,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.collection.d.ts(63,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.iterable.d.ts(162,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.collection.d.ts(63,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.iterable.d.ts(162,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.collection.d.ts(63,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.iterable.d.ts(162,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.collection.d.ts(63,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.iterable.d.ts(162,11)
- /home/vsts/work/1/s/typescript-54788/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
[MISSING] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
- /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)

typescript-bot · 2023-06-26T23:01:25Z

Hey @DanielRosenwasser, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

typescript-bot · 2023-06-27T00:21:16Z

@DanielRosenwasser Here are the results of running the top-repos suite comparing main and refs/pull/54788/merge:

Something interesting changed - please have a look.

Details

Server exited prematurely with code unknown and signal SIGABRT

Server exited prematurely with code unknown and signal SIGABRT

Affected repos

redwoodjs/redwood

Raw error text: RepoResults4/redwoodjs.redwood.rawError.txt in the artifact folder

Last few requests

{"seq":1906,"type":"request","command":"getOutliningSpans","arguments":{"file":"@PROJECT_ROOT@/packages/auth-providers/azureActiveDirectory/web/jest.config.js"}}
{"seq":1907,"type":"request","command":"navto","arguments":{"searchValue":"a","maxResultCount":256}}
{"seq":1908,"type":"request","command":"navto","arguments":{"searchValue":"A_E","maxResultCount":256}}
{"seq":1909,"type":"request","command":"definitionAndBoundSpan","arguments":{"file":"@PROJECT_ROOT@/packages/auth-providers/azureActiveDirectory/web/jest.config.js","line":1,"offset":5}}

Repro steps

git clone https://github.com/redwoodjs/redwood --recurse-submodules
In dir redwood, run git reset --hard 625629bedfa7226269ee9d9e212bcfbc217d944f
Install packages (exact steps are below, but it might be easier to follow the repo readme)
1. In dir redwood/docs, run yarn install --no-immutable --mode=skip-build
2. In dir redwood, run yarn install --no-immutable --mode=skip-build
3. In dir redwood/tasks/check, run yarn install --no-immutable --mode=skip-build
Back in the initial folder, download RepoResults4/redwoodjs.redwood.replay.txt from the artifact folder
npm install --no-save @typescript/server-replay
npx tsreplay ./redwood ./redwoodjs.redwood.replay.txt path/to/tsserver.js
npx tsreplay --help to learn about helpful switches for debugging, logging, etc

src/testRunner/unittests/helpers/node10Result.ts

Co-authored-by: Sheetal Nandi <[email protected]>

typescript-bot · 2023-06-27T21:20:54Z

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.\n\nAlso, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

jakebailey · 2023-06-27T21:27:01Z

\n\n

Seriously? Well, at least the bot is confirmed working...

sheetalkamat · 2023-06-27T21:58:02Z

tests/baselines/reference/tscWatch/moduleResolution/node10Result.js

+
+[91merror[0m[90m TS2318: [0mCannot find global type 'String'.
+
+[91merror[0m[90m TS6053: [0mFile '/a/lib/lib.es2022.full.d.ts' not found.


Oh man.. this is getting tricky,. i suggested wrong lib file? :( Sorry

The diff is so hard to look at locally because of the terminal color codes in the actual content, so I’m not paying too much attention to what I’m pushing 😅 Easier to look on GH

src/testRunner/unittests/helpers/node10Result.ts

Co-authored-by: Sheetal Nandi <[email protected]>

weswigham · 2023-07-11T14:00:53Z

...aselines/reference/conditionalExportsResolutionFallback(moduleresolution=bundler).errors.txt

@@ -1,7 +1,9 @@
 error TS5095: Option 'bundler' can only be used when 'module' is set to 'es2015' or later.
+error TS5109: Option 'moduleResolution' must be set to 'NodeNext' (or left unspecified) when option 'module' is set to 'NodeNext'.


 !!! error TS5095: Option 'bundler' can only be used when 'module' is set to 'es2015' or later.


Wait... so module now controls weather we interpret files as cjs/esm, but you still can't combine that with bundler? I thought the point of this was basically to make it so bundler could assimilate the mode-swapping interpretation? So you could mimic bundlers that more closely mimiced node?

Isn't allowing module: nodenext + moduleResultion: bundler for modern-node-like bundler environments kinda the goal here?

(Since this format data directly controls, eg, how we interpret default imports, the distinction may be material)

Yeah, that's the linked issue #54102 - shouldn't we just fix that, while we're here? It should be removing this error, right?

Yeah, that is the ultimate goal, but I don’t think --module nodenext is the right thing to combine with --moduleResolution bundler. --module nodenext implies that CJS-mode files actually get emitted as CJS, which doesn’t make sense in the context of most bundlers. In bundlers, the choice is not between CJS and ESM; the default is ESM with Babel-like interop and .mjs files just disable the Babel-like interop in favor of Node’s always-synthesize-a-default interop. It can be hard to tell the difference between “ESM with Babel-like interop” and “it’s actually just CJS with esModuleInterop under the hood,” but we can tell there’s a difference because

top-level wait is always allowed (by those that support it at all)

the module format does not affect the "imports"/"exports" conditions used in module resolution—import statements always have "import"; they’re not secretly transformed to require first in CJS-mode files

some bundlers refuse to process require calls outside of node_modules at all

Additionally, the transform for import foo = require(...) to a createRequire call under --module nodenext is highly Node-specific and would fail under I think every bundler (maybe Webpack has compat for that; they do the most complete job at imitating Node).

So, my thinking for addressing #54102 is that either there needs to be a different module mode for these bundlers that have Node-like interop behavior (in which case this PR is necessary but not sufficient), or the thing that makes us set impliedNodeFormat in the first place needs to be decoupled from module (in which case this PR is not necessary, but doesn’t hurt anything as an interim step toward the eventual goal). Either way, we’ll need to move it off of moduleResolution in order to resolve #54102, and module will imply an impliedNodeFormat-setting behavior if it doesn’t directly control it.

andrewbranch added 2 commits June 26, 2023 11:45

Make module, not moduleResolution, the trigger for setting impliedNod…

660bf60

…eFormat

Update baselines

77a931b

typescript-bot assigned andrewbranch Jun 26, 2023

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 26, 2023

Reset target for less baseline change

081a381

andrewbranch force-pushed the module-resolution-is-just-resolution branch from 8118262 to 081a381 Compare June 26, 2023 20:03

andrewbranch commented Jun 26, 2023

View reviewed changes

tests/baselines/reference/tsc/moduleResolution/node10Result.js Show resolved Hide resolved

src/testRunner/unittests/tscWatch/forceConsistentCasingInFileNames.ts Outdated Show resolved Hide resolved

andrewbranch requested a review from weswigham June 26, 2023 20:06

sheetalkamat reviewed Jun 26, 2023

View reviewed changes

src/testRunner/unittests/helpers/node10Result.ts Outdated Show resolved Hide resolved

Set lib file content instead of changing target

d3c08b2

andrewbranch force-pushed the module-resolution-is-just-resolution branch from 84a06c3 to d3c08b2 Compare June 26, 2023 20:56

Replace lib file instead of changing target

34cf76c

andrewbranch requested a review from sheetalkamat June 26, 2023 21:31

sheetalkamat requested changes Jun 27, 2023

View reviewed changes

src/testRunner/unittests/helpers/node10Result.ts Outdated Show resolved Hide resolved

src/testRunner/unittests/helpers/node10Result.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

46572b0

Co-authored-by: Sheetal Nandi <[email protected]>

Update node10Result baselines

f346be4

jakebailey mentioned this pull request Jun 27, 2023

Fix newlines in PR bot reply #54803

Merged

sheetalkamat reviewed Jun 27, 2023

View reviewed changes

src/testRunner/unittests/helpers/node10Result.ts Outdated Show resolved Hide resolved

andrewbranch and others added 2 commits June 27, 2023 16:08

Update src/testRunner/unittests/helpers/node10Result.ts

4e35976

Co-authored-by: Sheetal Nandi <[email protected]>

Fix node10Result baselines

4d47e76

sheetalkamat approved these changes Jun 28, 2023

View reviewed changes

weswigham reviewed Jul 11, 2023

View reviewed changes

andrewbranch mentioned this pull request Jul 31, 2023

Rethinking module for the present and the future #55221

Open

andrewbranch mentioned this pull request Mar 21, 2024

Respect package.json "type" and module-format-specific file extensions in more module modes #57896

Merged

typescript-bot added the Fix Available A PR has been opened for this issue label Mar 21, 2024

andrewbranch closed this in #57896 Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `module` control `impliedNodeFormat` and `moduleResolution` control just module resolution #54788

Make `module` control `impliedNodeFormat` and `moduleResolution` control just module resolution #54788

andrewbranch commented Jun 26, 2023

DanielRosenwasser commented Jun 26, 2023

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023

typescript-bot commented Jun 26, 2023

rxjs-src

/mnt/ts_downloads/rxjs-src/build.sh

typescript-bot commented Jun 26, 2023

typescript-bot commented Jun 27, 2023

Server exited prematurely with code unknown and signal SIGABRT

Affected repos

Last few requests

Repro steps

typescript-bot commented Jun 27, 2023

jakebailey commented Jun 27, 2023

sheetalkamat Jun 27, 2023

andrewbranch Jun 27, 2023

weswigham Jul 11, 2023

weswigham Jul 11, 2023

weswigham Jul 11, 2023

andrewbranch Jul 11, 2023


		[91merror[0m[90m TS2318: [0mCannot find global type 'String'.

		[91merror[0m[90m TS6053: [0mFile '/a/lib/lib.es2022.full.d.ts' not found.

Make module control impliedNodeFormat and moduleResolution control just module resolution #54788

Make module control impliedNodeFormat and moduleResolution control just module resolution #54788

Conversation

andrewbranch commented Jun 26, 2023

DanielRosenwasser commented Jun 26, 2023

typescript-bot commented Jun 26, 2023 • edited Loading

typescript-bot commented Jun 26, 2023 • edited Loading

typescript-bot commented Jun 26, 2023 • edited Loading

typescript-bot commented Jun 26, 2023 • edited Loading

typescript-bot commented Jun 26, 2023 • edited Loading

typescript-bot commented Jun 26, 2023 • edited Loading

typescript-bot commented Jun 26, 2023 • edited Loading

typescript-bot commented Jun 26, 2023 • edited Loading

typescript-bot commented Jun 26, 2023

typescript-bot commented Jun 26, 2023

rxjs-src

/mnt/ts_downloads/rxjs-src/build.sh

typescript-bot commented Jun 26, 2023

typescript-bot commented Jun 27, 2023

Server exited prematurely with code unknown and signal SIGABRT

Affected repos

Last few requests

Repro steps

typescript-bot commented Jun 27, 2023

jakebailey commented Jun 27, 2023

sheetalkamat Jun 27, 2023

Choose a reason for hiding this comment

andrewbranch Jun 27, 2023

Choose a reason for hiding this comment

weswigham Jul 11, 2023

Choose a reason for hiding this comment

weswigham Jul 11, 2023

Choose a reason for hiding this comment

weswigham Jul 11, 2023

Choose a reason for hiding this comment

andrewbranch Jul 11, 2023

Choose a reason for hiding this comment

Make `module` control `impliedNodeFormat` and `moduleResolution` control just module resolution #54788

Make `module` control `impliedNodeFormat` and `moduleResolution` control just module resolution #54788

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading

typescript-bot commented Jun 26, 2023 •

edited

Loading