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

Compatibility with old TS libraries under NodeNext resolution #50482

Closed
5 tasks done
Jack-Works opened this issue Aug 27, 2022 · 7 comments
Closed
5 tasks done

Compatibility with old TS libraries under NodeNext resolution #50482

Jack-Works opened this issue Aug 27, 2022 · 7 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@Jack-Works
Copy link
Contributor

Suggestion

Improve the resolution for old ts libraries.

🔍 Search Terms

NodeNext node_modules

✅ Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

Many libraries ships .d.ts without .js extension and mark them as type: "module".

export { a } from './file'
//                ~~~~~~~~ should be ./file.js

TypeScript now treat them as error and refuse to resolve the type. This makes it even harder to adopt the new resolution mode.

📃 Motivating Example

Example: https://github.com/Jack-Works/ts-nodenext-legacy-library-resolution

node_modules/invalid-node-esm is a very common pattern in the current ecosystem. TypeScript failed to resolve this package as the following message:

node_modules/invalid-node-esm/dist/index.d.ts:1:15 - error TS2835: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean './utils.js'?

1 export * from './utils'
                ~~~~~~~~~

src/index.ts:1:10 - error TS2305: Module '"invalid-node-esm"' has no exported member 'a'.

1 import { a } from 'invalid-node-esm'
           ~

This behavior is correct, but this is not usable.

💻 Use Cases

Improve the compatibility and the adoption of the new resolution mode.

@Jack-Works
Copy link
Contributor Author

@andrewbranch
Copy link
Member

We are not going to perform invalid resolutions, but I would be open to improving the error message for ESM resolution errors when the target is in node_modules.

Did you mean './utils.js'?

Because clearly you didn’t mean anything; the author of the package did. If you or someone else wants to open a PR, I can help with the wording. But the suggestion to just fall back to wrong resolutions is declined.

@Jack-Works
Copy link
Contributor Author

Note: In order to show the message Did you mean './utils.js'? in a library, I use the skipLibCheck: false flag.

By default, it only has the following message and it is a total mystery to the developers. They may don't have the knowledge about all this Node ESM + TypeScript and think "oh, it just not working at all, let's forget it."

src/index.ts:1:10 - error TS2305: Module '"invalid-node-esm"' has no exported member 'a'.

1 import { a } from 'invalid-node-esm'
           ~

To emit the correct declaration resolvable under NodeESM mode, it requires library authors to totally reconfigure their build system and republish the package, and it is even harder if the library author wants to support both CommonJS and ES Module.

@andrewbranch
Copy link
Member

To emit the correct declaration resolvable under NodeESM mode

Before we worry about TypeScript resolving types, we need to worry about Node resolving JS. Making a dual CJS/ESM package is harder than making a CJS package. Node’s module support is complicated, even without TypeScript. The nice thing about TypeScript’s approach is once you have everything it takes to satisfy Node, literally all you have to do to satisfy TypeScript is put .d.[mc]ts files next to the .[mc]js files. This is why we absolutely cannot loosen our module resolution algorithm to be more forgiving—it’s not our algorithm; it’s not our choice. We have to reflect what Node is going to do, or else we will tell you everything is fine when Node is going to crash at runtime.

If node_modules/invalid-node-esm/dist/index.d.ts says export { a } from './file', that means that node_modules/invalid-node-esm/dist/index.js says export { a } from './file', which will crash in Node in a "type": "module" scope. If we don’t give you an error on that, we’re not protecting you from legitimately broken imports. If the declaration file is using an import path that the JS file is not, the declaration file is wrong. We have no more right to assume a module specifier in .d.ts file is wrong and fix it up than we have to transparently change other types in .d.ts files when they would cause errors. “Oh, this unknown in foo.d.ts is causing your code to have an error? The author was probably mistaken; let’s just change it to any so the error goes away.” 🤨

@andrewbranch andrewbranch added Suggestion An idea for TypeScript In Discussion Not yet reached consensus Domain: Error Messages The issue relates to error messaging labels Sep 7, 2022
@a-type

This comment was marked as outdated.

@andrewbranch
Copy link
Member

@a-type two things:

  1. Vite is more or unless unsupported by TypeScript right now. How much trouble you have using various workarounds Just Depends™. See A Proposal For Module Resolution #50152 and Support package.json exports outside of Node #50794 for some details.
  2. @stitches/react is misconfigured; I think you started to figure this out. Their package.json says "type": "module" but their .d.ts files use extensionless imports. Analyzing exactly what they should do instead is frankly a lot of work, but I can confirm it’s expected behavior that it’s not working for you under nodenext. At a glance, it looks like things would probably Just Work™ if their .d.ts files all used .js extensions in their imports, but the real happy path for dual CJS/ESM packages is to have dual CJS/ESM type declarations that can accurately represent the nuances between the two modes rather than trying to massage a single set of definitions into a format that mostly works for both modes—but that is sometimes possible, particularly when default exports aren’t used.

@andrewbranch
Copy link
Member

I guess for clarity, I’m going to close this rather than retitle it to make it match my verdict of “no, but we’ll update error messages.” I would take a PR for error messages, and @Jack-Works if you want to open another one for that, that’s fine. But I don’t want to leave this open and risk folks thinking there is any possible action we can take that addresses the title of the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants