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

Bug: Typescript(4.6+) moduleResolve mode node12 nodenext do not shadow package.json filds types and main #48235

Closed
frank-dspeed opened this issue Mar 13, 2022 · 8 comments
Assignees
Labels
External Relates to another program, environment, or user action which we cannot control.

Comments

@frank-dspeed
Copy link

frank-dspeed commented Mar 13, 2022

Bug Report

In the NodeJS resolve spec is mentioned that the main field and all other alias fields like browser modules
get shadowed by exports so there is a fallback if a field is missing it looks up the original fields Typescript 4.6 does not do so and so I am forced to split dependencies for consumption into the once that support package.json exports: field including all sub dependencies.

see: #46452 (comment)

for me that is not a big issue as i tend to create dev bundels any way but most people will not have a good time without that fallback

see: #46452

🔎 Search Terms

node12 nodenext moduleResolve

🕗 Version & Regression Information

4.5 + new resolveModes

🙁 Actual behavior

I Started Consuming libs with node12 and nodenext resolve mode and got a lot of errors then I looked into the packages to identify what they did and found out there is no fallback in typescript which helped me to identify packages fast that got no exports field or export fields with subPath patterns as they are also not supported.

🙂 Expected behavior

always remember the rule that is most specific is the one that gets in effect! Like with css when a selector is more specific it gets used.
lookup patterns nodenext :

  • package.json => exports: types:,
  • package.json => types:,
  • package.json => exports: node?.import|import|default,
  • package.json => main:

same for node12

Short

moduleResolve modes node12 and nodenext should be backward compatible and should inhire resolve mode node which also includes Classic

Related Partial (Classic)

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Mar 14, 2022
@weswigham
Copy link
Member

weswigham commented Mar 14, 2022

You seem to be under this mistaken impression that only one condition is active at a time and that we could execute multiple lookups inside the export map. Unfortunately, this is wrong, because compound conditions exist. You must resolve all export map conditions in a single pass to respect its export-blocking behavior, and that includes our types condition. The lack of a types condition does not imply a fallback to legacy main or types for lookup - on the contrary, it means that either the types are either alongside the provided export map entries or that there are no types at all; to do anything else is to usurp the intended blocking behavior of an export map.

Every package author was well-informed that adding an export map is a breaking change - one of those breaks is to TS, unless you provide appropriate types entrypoints.

@weswigham weswigham added External Relates to another program, environment, or user action which we cannot control. and removed Needs Investigation This issue needs a team member to investigate its status. labels Mar 14, 2022
@weswigham
Copy link
Member

In short: if you encounter any package with a types package json entry but not types in a provided export map - report it as an issue on that package; they've (likely unintentionally) broken TS support.

@frank-dspeed
Copy link
Author

@weswigham i guess your missing a little word because my english is so bad the exports map does shadow shadow means it takes the value of the existing fild if it exists and is not in the exports map also that is not a export map this does not fully implement the importMaps Proposal it is simply a temp addition of that feature so the algo should be designed like

  • single lookup ok!
  • get package.json read level 1
  • if level 1 got exports add all missing filds from level 1 to exports

then do the lookup the single one
that is meaned by design when we write the filds get shadowed its like with the Shadow DOM in the browser it is a overlay a onion type.

@frank-dspeed
Copy link
Author

frank-dspeed commented Mar 15, 2022

@guybedford can you please verify my assumption or enlighten me? It would help me a lot. And if i am correct maybe point to something that defines that in better english i did not find the related documentation.

@guybedford
Copy link
Contributor

The implementation @weswigham describes is correct in that "exports" is authoritative and there is no fallback. There is one fallback exception though and that is that internal browser mappings do chain with the exports field:

{
  "exports": "./x.js"
  "browser": {
    "./x.js": "./x-browser.js"
  }
}

This was first implemented by Webpack then picked up by other bundlers so has become a convention now and I personally went along with it as well.

In all other cases, the exports field doesn't fall back at all to checking other fields.

That said, since TypeScript "types" usually correspond 1-1 with JS files, perhaps @weswigham there is a way to fall back to checking the expected "import" / "module" condition with a .d.ts extension in place of the .mjs or .js extension. It seems like this would fix the usability issue in the example described without needing a complete verbose type listing for every export.

@frank-dspeed
Copy link
Author

frank-dspeed commented Mar 15, 2022

@guybedford @weswigham what do you think about that i droped that idea as i tought that would be a more aggressiv way but your current suggestion sounds a bit like that and it is a valid way to solve compat issues. #48239

@weswigham
Copy link
Member

weswigham commented Mar 15, 2022

perhaps @weswigham there is a way to fall back to checking the expected "import" / "module" condition with a .d.ts extension in place of the .mjs or .js extension.

Already works that way (.d.mts corresponds to .mjs, however). :)

@frank-dspeed
Copy link
Author

ok lets close that i will simply finish the package test utils and then we can see the results and then issue pull requests via the nodejs package maintainance group we have all moving parts in place i guess. So simply lets see what happens. Thx to all for making sure that the ecosystem evolves more fast.

wight554 pushed a commit to wight554/htm that referenced this issue Apr 25, 2022
wight554 added a commit to wight554/Recoil that referenced this issue Apr 25, 2022
facebook-github-bot pushed a commit to facebookexperimental/Recoil that referenced this issue May 5, 2022
Summary:
* as per microsoft/TypeScript#48235 (comment)

Pull Request resolved: #1753

Reviewed By: mondaychen

Differential Revision: D36181630

Pulled By: drarmstr

fbshipit-source-id: 6a3edf1dbdb03e32d62acb89008bb3d0e626837b
AlexGuz23 pushed a commit to AlexGuz23/Recoil that referenced this issue Nov 3, 2022
Summary:
* as per microsoft/TypeScript#48235 (comment)

Pull Request resolved: facebookexperimental/Recoil#1753

Reviewed By: mondaychen

Differential Revision: D36181630

Pulled By: drarmstr

fbshipit-source-id: 6a3edf1dbdb03e32d62acb89008bb3d0e626837b
snipershooter0701 pushed a commit to snipershooter0701/Recoil that referenced this issue Mar 5, 2023
Summary:
* as per microsoft/TypeScript#48235 (comment)

Pull Request resolved: facebookexperimental/Recoil#1753

Reviewed By: mondaychen

Differential Revision: D36181630

Pulled By: drarmstr

fbshipit-source-id: 6a3edf1dbdb03e32d62acb89008bb3d0e626837b
eagle2722 added a commit to eagle2722/Recoil that referenced this issue Sep 21, 2024
Summary:
* as per microsoft/TypeScript#48235 (comment)

Pull Request resolved: facebookexperimental/Recoil#1753

Reviewed By: mondaychen

Differential Revision: D36181630

Pulled By: drarmstr

fbshipit-source-id: 6a3edf1dbdb03e32d62acb89008bb3d0e626837b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Relates to another program, environment, or user action which we cannot control.
Projects
None yet
Development

No branches or pull requests

4 participants