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

unstable_enablePackageExports does not resolve the root '.' exports alias #965

Comments

@shamilovtim
Copy link

Do you want to request a feature or report a bug?
Bug

What is the current behavior?

  "exports": {
    ".": {
      "types": "./dist/src/index.d.ts",
      "import": "./dist/src/index.js"
    },
    "./chunker": {
      "types": "./dist/src/chunker/index.d.ts",
      "import": "./dist/src/chunker/index.js"
    },
    "./layout": {
      "types": "./dist/src/layout/index.d.ts",
      "import": "./dist/src/layout/index.js"
    }
  }

metro is 0.76.0
Given that unstable_enablePackageExports is true.
Given the above exports in a package.json . It seems that metro does not recognize that . is the main package. I receive the following error:

error: Error: While trying to resolve module `ipfs-unixfs-importer` from file `/Users/admin/Documents/Development/block/rc1wallet/node_modules/@tbd54566975/dwn-sdk-js/dist/esm/src/store/data-store-level.js`, the package `/Users/admin/Documents/Development/block/rc1wallet/node_modules/ipfs-unixfs-importer/package.json` was successfully found. However, this package itself specifies a `main` module field that could not be resolved (`/Users/admin/Documents/Development/block/rc1wallet/node_modules/ipfs-unixfs-importer/index`. Indeed, none of these files exist:

  * /Users/admin/Documents/Development/block/rc1wallet/node_modules/ipfs-unixfs-importer/index(.native|.ios.js|.native.js|.js|.ios.jsx|.native.jsx|.jsx|.ios.json|.native.json|.json|.ios.ts|.native.ts|.ts|.ios.tsx|.native.tsx|.tsx)
  * /Users/admin/Documents/Development/block/rc1wallet/node_modules/ipfs-unixfs-importer/index/index(.native|.ios.js|.native.js|.js|.ios.jsx|.native.jsx|.jsx|.ios.json|.native.json|.json|.ios.ts|.native.ts|.ts|.ios.tsx|.native.tsx|.tsx)
    at DependencyGraph.resolveDependency (/Users/admin/Documents/Development/block/rc1wallet/node_modules/metro/src/node-haste/DependencyGraph.js:283:17)
    at Object.resolve (/Users/admin/Documents/Development/block/rc1wallet/node_modules/metro/src/lib/transformHelpers.js:171:21)
    at Graph._resolveDependencies (/Users/admin/Documents/Development/block/rc1wallet/node_modules/metro/src/DeltaBundler/Graph.js:420:35)
    at Graph._processModule (/Users/admin/Documents/Development/block/rc1wallet/node_modules/metro/src/DeltaBundler/Graph.js:218:38)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Graph._addDependency (/Users/admin/Documents/Development/block/rc1wallet/node_modules/metro/src/DeltaBundler/Graph.js:316:20)
    at async Promise.all (index 2)
    at async Graph._processModule (/Users/admin/Documents/Development/block/rc1wallet/node_modules/metro/src/DeltaBundler/Graph.js:263:5)
    at async Graph._addDependency (/Users/admin/Documents/Development/block/rc1wallet/node_modules/metro/src/DeltaBundler/Graph.js:316:20)
    at async Promise.all (index 5)

If the current behavior is a bug, please provide the steps to reproduce and a minimal repository on GitHub that we can yarn install and yarn test.
https://github.com/shamilovtim/metro76exportsrepro
bug is present in package @tbd54566975/dwn-sdk-js

What is the expected behavior?
Expect metro to resolve "." : { } inside of exports and recognize that this provide the main resolution, which would lead it to finding index.js in the correct spot.

Please provide your exact Metro configuration and mention your Metro, node, yarn/npm version and operating system.

Metro config

const config = {
  resolver: {
    unstable_enablePackageExports: true,
    unstable_conditionNames: ['react-native', 'require', 'import'],
  },
};

module.exports = mergeConfig(getDefaultConfig(__dirname), config);

metro 0.76.0
yarn 1.22.18
node v16.18.0
npm 9.4.2
macOS latest

@huntie
Copy link
Member

huntie commented Apr 7, 2023

Thanks for the bug report! Will investigate next week 👀

  • We have an existing test case for conditional exports on the main entry point today, and this is working inside Meta's codebase — so I think this might be failing for a more complex reason here.
  • On a quick attempt at reproing this (thanks for sharing) against Metro's main branch (which has some recent fixes clearing out the incorrect @babel/runtime warnings), I'm unable to replicate the issue — but will look deeper to make sure.

@shamilovtim
Copy link
Author

@huntie is resolverMainFields not compatible with unstable_enablePackageExports? It seems when I try to configure these two together, for example with:

const config = {
  resolver: {
    resolverMainFields: ['react-native', 'main'],
    unstable_enablePackageExports: true,
    unstable_conditionNames: ['react-native', 'require', 'import'],
  },
};

This breaks bundling for me and exports is no longer usable. Adding exports manually to resolverMainFields creates a different class of error in my application. I think this could be related to the source of my original bug.

@shamilovtim
Copy link
Author

Updating https://github.com/shamilovtim/metro76exportsrepro with the latest repro demonstrating my above comment. After all of the babel warnings you should receive the following error:

error: Error: While trying to resolve module `ipfs-unixfs-importer` from file `/Users/admin/Documents/Development/block/rc1wallet/node_modules/@tbd54566975/dwn-sdk-js/dist/esm/src/store/data-store-level.js`, the package `/Users/admin/Documents/Development/block/rc1wallet/node_modules/ipfs-unixfs-importer/package.json` was successfully found. However, this package itself specifies a `main` module field that could not be resolved (`/Users/admin/Documents/Development/block/rc1wallet/node_modules/ipfs-unixfs-importer/index`. Indeed, none of these files exist:

@huntie
Copy link
Member

huntie commented Apr 11, 2023

is resolverMainFields not compatible with unstable_enablePackageExports?

These are independent options. If unstable_enablePackageExports is enabled, and the package has an "exports" field, then only unstable_conditionNames is considered.

However if that fails, then we fall back to resolverMainFields in a second pass. (Note: We have incoming docs that will describe this behaviour! You can dig into the RFC in the meantime.)


Okay, I've reproduced the bug! It's related to how we locate a package.json for a given filePath (used by both "exports" and mainFields resolution). Skips over that package manifest entirely, at least for the "exports" resolution pass :|.

➡️ Fix incoming.

However, after fixing this, it's worth noting that other deps in your project seem to widely depend on Node.js APIs, which could independently be problematic.

  • classic-level — uses "main" only.
  • readable-stream — uses "browser" field — would need to be included in your resolverMainFields config.

image

@shamilovtim
Copy link
Author

shamilovtim commented Apr 11, 2023

is resolverMainFields not compatible with unstable_enablePackageExports?

These are independent options. If unstable_enablePackageExports is enabled, and the package has an "exports" field, then only unstable_conditionNames is considered.

However if that fails, then we fall back to resolverMainFields in a second pass. (Note: We have incoming docs that will describe this behaviour! You can dig into the RFC in the meantime.)

Okay, I've reproduced the bug! It's related to how we locate a package.json for a given filePath (used by both "exports" and mainFields resolution). Skips over that package manifest entirely, at least for the "exports" resolution pass :|. (Update: This was previously inconsequential for mainFields, since redirections only support subpaths. This bug is entirely valid as reported for "exports"! ✅)

➡️ Fix incoming.

Awesome, glad you found it!

However, after fixing this, it's worth noting that other deps in your project seem to widely depend on Node.js APIs, which could independently be problematic.

  • classic-level — uses "main" only.
  • readable-stream — uses "browser" field — would need to be included in your resolverMainFields config.
image

Appreciate you pointing that out. I am aware of it and it's a huge pain in the butt. I'm asking the upstream guys to separate out the node implementation so I can get one that doesn't include stuff like classic-level. Readable Stream should be polyfilled but the polyfill seems to depend on node's readable-stream, which is strange. I hope to get this sorted out in the next couple of days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment