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

[Case Study] Exports Support #650

Closed
1 task
sparebytes opened this issue Dec 26, 2019 · 4 comments · Fixed by #2431
Closed
1 task

[Case Study] Exports Support #650

sparebytes opened this issue Dec 26, 2019 · 4 comments · Fixed by #2431
Labels
case study Package compatibility report help wanted Extra attention is needed

Comments

@sparebytes
Copy link

sparebytes commented Dec 26, 2019

  • I'd be willing to implement a fix

Describe the bug

See:

Node ^12.7.0 with the --experimental-modules flag and Node ^13.0.0 without the flag resolves modules differently than Yarn@2.0.0-rc19 with PNP

To Reproduce

See this project:
https://github.com/sparebytes/yarn2-pnp-exports-example

// packages/package-with-exports/package.json
{
    "name": "package-with-exports",
    "version": "1.0.0",
    "exports": {
        ".": "./main.js",
        "./sub/other": "./other.js"
    }
}

// Example 1
// Expected: packages/package-with-exports/main.js
// Actual: packages/package-with-exports/index.js
require("package-with-exports");

// Example 2
// Expected: packages/package-with-exports/other.js
// Actual: throws error
require("package-with-exports/sub/other");

Environment

  • OS: Windows
  • Node version ^12.7.0
  • Yarn version 2.0.0-rc19
@sparebytes sparebytes added the bug Something isn't working label Dec 26, 2019
@arcanis arcanis added the help wanted Extra attention is needed label Jan 3, 2020
@sparebytes
Copy link
Author

sparebytes commented Jan 21, 2020

@jkrems
Copy link

jkrems commented Apr 15, 2020

If nodejs/node#32869 lands, this likely should include support for NODE_ENV based resolution ("production" and "development").

@bgotink
Copy link
Sponsor Member

bgotink commented Apr 27, 2020

This one is hard to fit in the current PnP setup. Resolution is split into two parts:

  • resolveToUnqualified
  • resolveUnqualified

and neither is (at the moment) a good fit for package exports.

Why isn't `resolveToUnqualified` a good place for package export resolution?

Two big reasons:

  • It requires reading from the package manifest, and resolveToUnqualified is not meant to touch the file system.
  • The current behaviour of the function is to do a naive resolution. In other words: if I resolve pkg, I will get a result looking like /path/to/.yarn/cache/pkg-hash.zip/node_modules/pkg. If we add package export resolution here, that result will change to /path/to/.yarn/cache/pkg-hash.zip/node_modules/pkg/index.js assuming the pkg package has a main index.js export.
Why isn't `resolveUnqualified` a good place for package export resolution?

This is best shown using an example. Suppose we've got a package with manifest

{
  "name": "exported",
  "main": "./index.js",
  "exports": {
    "default": "./index.js",
    "./deep": "./lib/deep.js"
  }
}

The package contains three javascript files:

  • index.js
  • other.js
  • lib/deep.js

If we implement package export resolution in resolveUnqualified we expect the following behaviour:

resolving: "exported"
resolveToUnqualified: /path/to/.yarn/cache/exported-hash.zip/node_modules/exported
resolveUnqualified: /path/to/.yarn/cache/exported-hash.zip/node_modules/exported/index.js

resolving: "exported/deep"
resolveToUnqualified: /path/to/.yarn/cache/exported-hash.zip/node_modules/exported/deep
resolveUnqualified: /path/to/.yarn/cache/exported-hash.zip/node_modules/exported/lib/deep.js

resolving: "./other" from the "exported" package's index.js file
resolveToUnqualified: /path/to/.yarn/cache/exported-hash.zip/node_modules/exported/other
resolveUnqualified: /path/to/.yarn/cache/exported-hash.zip/node_modules/exported/lib/other.js

resolving: "exported/other"
resolveToUnqualified: /path/to/.yarn/cache/exported-hash.zip/node_modules/exported/other
resolveUnqualified: this should throw because the "./other" export is not defined
  • As the example above shows, the resolveUnqualified function must be able to distinguish between importing a file via a bare identifier or via a relative path. Currently that's impossible, because resolveUnqualified gets the unqualified path, which is identical in both cases.
  • Notice how the path changed quite a bit when resolving exported/deep. Every path that is handed to resolveUnqualified will now result in us opening the package manifest to see whether the relative path passed in matches an export or not.

I currently see three options:

  • Implement this behaviour in resolveUnqualified and give that function the information it needs (the imported identifier)
  • Implement the behaviour in resolveToUnqualified and give that function the information it needs. In other words: add package exports (and main?) to the PnP data.
  • Create a new function resolveExport that's called in between resolveToUnqualified and resolveUnqualified iff the package has defined exports. Whether a package has exports could be added to PnP data, or it could be read from the manifest. We could even store exports in the pnp data. If we decide not to store it in PnP data, we could cache the data so as not to load it anew for every resolution.

I'm inclined not to use the first option, it doesn't quite fit the description of that function.
The easiest solution would be the third option, reading data from the manifests (and caching?).

@yarnbot

This comment has been minimized.

@yarnbot yarnbot added the stale Issues that didn't get attention label Jun 9, 2020
@paul-soporan paul-soporan added case study Package compatibility report upholded Real issues without formal reproduction and removed bug Something isn't working stale Issues that didn't get attention upholded Real issues without formal reproduction labels Jun 9, 2020
@paul-soporan paul-soporan changed the title [Bug] PNP module resolution should honor proposal-pkg-exports [Case Study] Exports Support Jun 9, 2020
@merceyz merceyz self-assigned this Dec 22, 2020
@merceyz merceyz removed their assignment Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
case study Package compatibility report help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants
@jkrems @bgotink @arcanis @merceyz @sparebytes @paul-soporan @yarnbot and others