Skip to content
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

Module nodenext is resolving to cjs build incorrectly #1778

Closed
perrin4869 opened this issue May 31, 2022 · 7 comments · Fixed by #1782
Closed

Module nodenext is resolving to cjs build incorrectly #1778

perrin4869 opened this issue May 31, 2022 · 7 comments · Fixed by #1782
Labels
Milestone

Comments

@perrin4869
Copy link

Search Terms

nodenext

Expected Behavior

tsc and ts-node resolve to the same modules when using nodenext.

Actual Behavior

ts-node is resolving to the commonjs build of a dependency, even though tsc resolves to the esm build.

Steps to reproduce the problem

Using the reproduction below, notice:

11:34:45 {master} ~/ts-node-nodenext-bug$ npm install
11:35:13 {master} ~/ts-node-nodenext-bug$ npx tsc
11:35:28 (master) ~/ts-node-nodenext-bug$ node --loader ts-node/esm index.ts
(node:25884) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
/home/perrin4869/ts-node-nodenext-bug/node_modules/ts-node/src/index.ts:843
    return new TSError(diagnosticText, diagnosticCodes, diagnostics);
           ^
TSError: ⨯ Unable to compile TypeScript:
index.ts:3:7 - error TS2322: Type 'typeof import("/home/perrin4869/ts-node-nodenext-bug/foo/cjs/index")' is not assignable to type 'number'.

3 const bar: number = foo;
        ~~~

    at createTSError (/home/perrin4869/ts-node-nodenext-bug/node_modules/ts-node/src/index.ts:843:12)
    at reportTSError (/home/perrin4869/ts-node-nodenext-bug/node_modules/ts-node/src/index.ts:847:19)
    at getOutput (/home/perrin4869/ts-node-nodenext-bug/node_modules/ts-node/src/index.ts:1057:36)
    at Object.compile (/home/perrin4869/ts-node-nodenext-bug/node_modules/ts-node/src/index.ts:1411:41)
    at transformSource (/home/perrin4869/ts-node-nodenext-bug/node_modules/ts-node/src/esm.ts:400:37)
    at /home/perrin4869/ts-node-nodenext-bug/node_modules/ts-node/src/esm.ts:278:53
    at async addShortCircuitFlag (/home/perrin4869/ts-node-nodenext-bug/node_modules/ts-node/src/esm.ts:409:15)
    at async ESMLoader.load (node:internal/modules/esm/loader:407:20)
    at async ESMLoader.moduleProvider (node:internal/modules/esm/loader:326:11)
    at async link (node:internal/modules/esm/module_job:70:21) {
  diagnosticCodes: [ 2322 ]
}

I think this has to do with the fact that foo is using package.json with "type": "commonjs" to specify the commonjs build.
If this is changed to a cjs file, the problem does not occur

Minimal reproduction

https://github.com/perrin4869/ts-node-nodenext-bug

@cspotcode
Copy link
Collaborator

Thank you for the excellent reproduction.

As a workaround until a proper fix is made, you can add "transpileOnly": true to your tsconfig. It appears the runtime resolver hooks are working correctly, but the type-checker resolves incorrectly.

I tried adding "traceResolution": true to tsconfig so that I could see what typescript was logging as it discovered modules. It's strange, everything seems correct, except it doesn't explain how it decides to pick cjs vs esm. It just... picks wrong.

TSC output

❯ ./node_modules/.bin/tsc
...
======== Resolving module 'foo' from '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/index.ts'. ========
Module resolution kind is not specified, using 'NodeNext'.
File '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/package.json' exists according to earlier cached lookups.
Loading module 'foo' from 'node_modules' folder, target file type 'TypeScript'.
Found 'package.json' at '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/node_modules/foo/package.json'.
'package.json' does not have a 'typesVersions' field.
### NOTE this is the line where it chooses esm ###
File name '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/node_modules/foo/esm/index.js' has a '.js' extension - stripping it.
File '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/node_modules/foo/esm/index.ts' does not exist.
File '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/node_modules/foo/esm/index.tsx' does not exist.
File '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/node_modules/foo/esm/index.d.ts' exist - use it as a name resolution result.
Resolving real path for '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/node_modules/foo/esm/index.d.ts', result '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/foo/esm/index.d.ts'.
======== Module name 'foo' was successfully resolved to '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/foo/esm/index.d.ts'. ========

ts-node output

❯ node --loader ts-node/esm ./index.ts
...
======== Resolving module 'foo' from '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/index.ts'. ========
Module resolution kind is not specified, using 'NodeNext'.
Found 'package.json' at '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/package.json'.
'package.json' does not have a 'typesVersions' field.
Loading module 'foo' from 'node_modules' folder, target file type 'TypeScript'.
Found 'package.json' at '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/node_modules/foo/package.json'.
'package.json' does not have a 'typesVersions' field.
### NOTE this is the line where it chooses cjs. Why?  ###
File name '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/node_modules/foo/cjs/index.js' has a '.js' extension - stripping it.
File '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/node_modules/foo/cjs/index.ts' does not exist.
File '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/node_modules/foo/cjs/index.tsx' does not exist.
File '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/node_modules/foo/cjs/index.d.ts' exist - use it as a name resolution result.
Resolving real path for '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/node_modules/foo/cjs/index.d.ts', result '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/foo/cjs/index.d.ts'.
======== Module name 'foo' was successfully resolved to '/d/Personal-dev/@TypeStrong/ts-node-repros/ts-node-nodenext-bug/foo/cjs/index.d.ts'. ========
...

@cspotcode cspotcode added the bug label May 31, 2022
@cspotcode
Copy link
Collaborator

I think I found the issue. To support the new NodeNext mode, TypeScript's resolver needs to pass around new information about the type of the module making the import request: either cjs or esm. That info is passed in new, optional arguments in some of the compiler APIs, so it wasn't flagged as an error when we moved to the new TS version. Should be a straightforward fix that I can do soon.

@cspotcode
Copy link
Collaborator

Note to self, how to fix this bug:

Update the signatures of these two functions to include the new, optional arguments in TS4.7
Do a sanity-check that these changes are fully backwards-compatible so that we continue to behave against older TS versions.

/*
* NOTE:
* Older ts versions do not pass `redirectedReference` nor `options`.
* We must pass `redirectedReference` to newer ts versions, but cannot rely on `options`, hence the weird argument name
*/
const resolveModuleNames: TSCommon.LanguageServiceHost['resolveModuleNames'] =
(
moduleNames: string[],
containingFile: string,
reusedNames: string[] | undefined,
redirectedReference: TSCommon.ResolvedProjectReference | undefined,
optionsOnlyWithNewerTsVersions: TSCommon.CompilerOptions
): (TSCommon.ResolvedModule | undefined)[] => {
return moduleNames.map((moduleName) => {
const { resolvedModule } = ts.resolveModuleName(
moduleName,
containingFile,
config.options,
host,
moduleResolutionCache,
redirectedReference
);
if (resolvedModule) {
fixupResolvedModule(resolvedModule);
}
return resolvedModule;
});
};
// language service never calls this, but TS docs recommend that we implement it
const getResolvedModuleWithFailedLookupLocationsFromCache: TSCommon.LanguageServiceHost['getResolvedModuleWithFailedLookupLocationsFromCache'] =
(
moduleName,
containingFile
): TSCommon.ResolvedModuleWithFailedLookupLocations | undefined => {
const ret = ts.resolveModuleNameFromCache(
moduleName,
containingFile,
moduleResolutionCache
);
if (ret && ret.resolvedModule) {
fixupResolvedModule(ret.resolvedModule);
}
return ret;
};

@perrin4869
Copy link
Author

thank you so much for the quick response @cspotcode!

@cspotcode cspotcode added this to the 10.8.1 milestone May 31, 2022
cspotcode added a commit that referenced this issue Jun 1, 2022
… cjs or esm -- into account when resolving package.json "exports" (#1782)

* Add failing test

* fix typo in failing test

* fix bug

* fix test runIf filter

* fix

* fix

* lint
@perrin4869
Copy link
Author

@cspotcode Thank you so much for the fix! Was just wondering if by any chance you could cut a 10.8.1, and leave #1764 for a 10.8.2 release? Just real eager to be using nodenext XD

@cspotcode
Copy link
Collaborator

Yeah, absolutely. I can do that tomorrow morning.

@cspotcode
Copy link
Collaborator

@perrin4869 v10.8.1 is published, thanks for your patience, and please let me know if you hit any other issues.

https://github.com/TypeStrong/ts-node/releases/tag/v10.8.1

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Jun 17, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [ts-node](https://typestrong.org/ts-node) ([source](https://github.com/TypeStrong/ts-node)) | devDependencies | patch | [`10.8.0` -> `10.8.1`](https://renovatebot.com/diffs/npm/ts-node/10.8.0/10.8.1) |

---

### Release Notes

<details>
<summary>TypeStrong/ts-node</summary>

### [`v10.8.1`](https://github.com/TypeStrong/ts-node/releases/tag/v10.8.1)

[Compare Source](TypeStrong/ts-node@v10.8.0...v10.8.1)

**Fixed**

-   Fixed [#&#8203;1769](TypeStrong/ts-node#1769): source URLs in source map cache were malformed on Windows, affecting code coverage reports ([#&#8203;1769](TypeStrong/ts-node#1769), [#&#8203;1771](TypeStrong/ts-node#1771)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Fixed [#&#8203;1778](TypeStrong/ts-node#1778): typechecker was erronously resolving imports from ESM files as if they were from CJS files ([#&#8203;1778](TypeStrong/ts-node#1778), [#&#8203;1782](TypeStrong/ts-node#1782)) [@&#8203;cspotcode](https://github.com/cspotcode)

https://github.com/TypeStrong/ts-node/milestone/14

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1393
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants