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

Package Exports not working #1128

Closed
Bekaxp opened this issue Oct 31, 2023 · 11 comments
Closed

Package Exports not working #1128

Bekaxp opened this issue Oct 31, 2023 · 11 comments

Comments

@Bekaxp
Copy link

Bekaxp commented Oct 31, 2023

Do you want to request a feature or report a bug?
It could be a BUG or maybe I just need some help with the configuration.

What is the current behavior?

  • package A has defined exports in the package.json
  • The native app has package A as a dependency
  • The metro config has the unstable_enablePackageExports enabled
  • When we start the app yarn start or npm start it explodes with an error:
Error: While resolving module `package A`, the Haste package `package A` was found. However the module `native` could not be found within the package.

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.

What is the expected behavior?
I would expect that the packages are resolved correctly and the app starts without issues.

Please provide your exact Metro configuration and mention your Metro, node, yarn/npm version and operating system.
My metro config looks like this:

const { getDefaultConfig } = require("expo/metro-config");

module.exports = (() => {
  const config = getDefaultConfig(__dirname);

  const { resolver } = config;

  config.resolver = {
    ...resolver,
    unstable_enableSymlinks: true,
    unstable_enablePackageExports: true,
    unstable_conditionNames: ["import", "require"],
  };

  config.watchFolders = [".."];

  return config;
})();

I am running it on a MacOS Sonoma 14.0 (Apple M1 Pro)

Thank you very much for any tips and tricks to make it work :)

@MadeinFrance
Copy link

Reproducible example

@Bekaxp
Copy link
Author

Bekaxp commented Nov 8, 2023

Hi!

I think i discovered the issue and I have a proposal for a solution but I need your help!

The error is easily reproducible with the examples from the repo I posted above in the issue. The problem I found is in the metro-resolver package.

Whenever you have a module which is using the "new" exports way of exposing its content, like this in package.json:

"exports": {
    "./native": {
      "types": "./lib/native/index.d.ts",
      "import": "./lib/native/index.mjs",
      "require": "./lib/native/index.cjs"
    },
    "./web": {
      "types": "./lib/web/index.d.ts",
      "import": "./lib/web/index.mjs",
      "require": "./lib/web/index.cjs"
    },
    "./package.json": "./package.json"
  },

and then you are consuming this module like this:
import Whatever from '@dp/mypackage/native' OR import Whatever from '@dp/mypackage/web' for web in this case...

the metro-resolver => resolve.js will consider this as a "Haste Module" BUT IT IS NOT!!

My Solution
I created this additional method, which checks if the exports are defined in the package.json of the imported module and if the unstable_enablePackageExports flag is set. If that is the case then this module should not be considered as a "Haste Module" and that part of the code should be skipped...

My function:

function isExportedSource(context, moduleName) {
  if (context.unstable_enablePackageExports) {
    let packageName = moduleName;
    let packageJsonPath = context.resolveHastePackage(packageName);
    while (packageJsonPath == null && packageName && packageName !== ".") {
      packageName = _path.default.dirname(packageName);
      packageJsonPath = context.resolveHastePackage(packageName);
    }
    if (packageJsonPath == null) {
      return failedFor();
    }

    if (packageJsonPath) {
      const pkg = context.getPackageForModule(packageJsonPath);
      const exportsField = pkg?.packageJson.exports;
      const packageName = pkg?.packageJson.name;
      const exportName = moduleName.replace(packageName, '');

      return Object.keys(exportsField).some((item) => item.includes(exportName));
    }
  }

  return false;
}

and should be used in the condition which wraps the call for resolving Haste named modules:

const isExportedSrc = isExportedSource(context, normalizePath(realModuleName));

  if (context.allowHaste && !isDirectImport && !isExportedSrc) {
    const normalizedName = normalizePath(realModuleName);
    const result = resolveHasteName(context, normalizedName, platform);
    if (result.type === "resolved") {
      return result.resolution;
    }
  }

When I have this in place, all modules all resolved correctly and my app builds without issues :)

Any thoughts? Should I create a proposal PR for you guys? PLEASE let me know :) :)

Thank you very much!

@Bekaxp
Copy link
Author

Bekaxp commented Nov 8, 2023

Maybe @robhogan or @huntie could you have look please? If you think this actually could be the solution I can open a PR as well 🥸. Thank you very much for your help and contribution!

@Bekaxp
Copy link
Author

Bekaxp commented Nov 8, 2023

One additional important note...

If in the example above, I remove config.watchFolders = [".."]; option for metro, then it will look for packages only inside the current directory (single repo situation) and it will work. BUT if we have a monorepo, like in our case and we need to look for modules outside the app folder this problem re-occurs.

@kraenhansen
Copy link
Contributor

@Bekaxp I wanted to verify your fix: Where do I put / call the isExportedSource and isExportedSrc functions?

@Bekaxp
Copy link
Author

Bekaxp commented Nov 10, 2023

Hi @kraenhansen, thanks for trying it out.

Once you install all the dependencies of the repro above, you should have in the node_modules a package named metro-resolver. In there you will find a file src/resolve.js. You should put that code in there. The isExportedSource should be checked in the condition for haste modules => if (context.allowHaste && !isDirectImport) {, which should be around line 90 in the resolve.js file.

I did some more testings with my function and is not perfect because actually some modules should be considered as haste and should enter into that condition. But at least I think it shows that there is a problem how Metro is trying to resolve modules in a monorepo.
Also, if you run a react-native project standalone (not in a monorepo) then the Metro resolver works correctly. This problem only happens when we have a monorepo and the files which needs to be resolved are all over the place 😃.

For now I solved the problem with exporting our packages in a different way (package.json example):

{
  "react-native": "./lib/native/index.mjs",
  "exports": {
    ".": {
      "types": "./lib/native/index.d.ts",
    },
    "./native": {
      "types": "./lib/native/index.d.ts",
      "import": "./lib/native/index.mjs",
      "require": "./lib/native/index.cjs"
    },
    "./web": {
      "types": "./lib/web/index.d.ts",
      "import": "./lib/web/index.mjs",
      "require": "./lib/web/index.cjs"
    },
    "./package.json": "./package.json"
  },
}

Of course then we need to import it from @dp/mypackage and not @dp/mypackage/native. Then Metro is happy and is able to resolve it correctly. But like I said it is just a workaround. I would really like to see Metro resolving it correctly.

Thank you again for trying it out!

@kraenhansen
Copy link
Contributor

kraenhansen commented Nov 10, 2023

I would really like to see Metro resolving it correctly.

I agree. As a React Native library developer, I'm experiencing the same issue.

I verified that simply disabling the if (context.allowHaste && !isDirectImport) branch (adding && false) in metro-resolver/src/resolve.js makes a package exports sub-path resolve correctly, which indicates that the PackageExportsResolve works as expected.

It definitely looks like this code is thoroughly tested by packages/metro-resolver/src/tests/package-exports-test.js. I've found context.allowHaste is true when the tests are running, but the result from returned by resolveHasteName is { type: 'failed', candidates: undefined } instead of it throwing.

@kraenhansen
Copy link
Contributor

I think I understand the issue better now. This happens only when the package being imported is in the haste map, because it's in the list of watchFolders.

I've added a failing test to main...kraenhansen:metro:fixing-1128 showing how this is broken and I'll try to see if I can contribute a fix for it too 🙏

@kraenhansen
Copy link
Contributor

Well, that wasn't too difficult: #1136.
Please take it for a spin to see if it fixes your issue too 🙂

@Bekaxp
Copy link
Author

Bekaxp commented Nov 10, 2023

@kraenhansen OMG!!! You did it! It is fixed! It works 😃😃😃. I will patch it for our projects but I guess the next version will already have your fix in place.

Should I close the issue or leave it open until your PR is merged in?

Thank you very much for this!

@kraenhansen
Copy link
Contributor

kraenhansen commented Nov 10, 2023

leave it open until your PR is merged in?

Please leave it open 🙂 I expect it to close as the PR merge 🙏

robhogan pushed a commit that referenced this issue Jan 9, 2024
…ame` (#1136)

Summary:
This fixes #1128 by calling the `resolvePackage` instead of `resolveModulePath` in `resolveHasteName`.
Only `resolvePackage` has the code to resolve package "exports" and it calls `resolveModulePath` as a fallback.

Changelog: [Experimental] When enabled, the `"exports"` field is now considered for Haste packages (which could include local monorepo packages)

Pull Request resolved: #1136

Test Plan: I've added a failing test which passed as the fix got implemented.

Reviewed By: huntie

Differential Revision: D51346769

Pulled By: robhogan

fbshipit-source-id: 8a003d5b147b73d344365db7cff8187ff946013d
robhogan pushed a commit that referenced this issue Jan 9, 2024
…ame` (#1136)

Summary:
This fixes #1128 by calling the `resolvePackage` instead of `resolveModulePath` in `resolveHasteName`.
Only `resolvePackage` has the code to resolve package "exports" and it calls `resolveModulePath` as a fallback.

Changelog: [Experimental] When enabled, the `"exports"` field is now considered for Haste packages (which could include local monorepo packages)

Pull Request resolved: #1136

Test Plan: I've added a failing test which passed as the fix got implemented.

Reviewed By: huntie

Differential Revision: D51346769

Pulled By: robhogan

fbshipit-source-id: 8a003d5b147b73d344365db7cff8187ff946013d
robhogan pushed a commit that referenced this issue Jan 30, 2024
…ame` (#1136)

Summary:
This fixes #1128 by calling the `resolvePackage` instead of `resolveModulePath` in `resolveHasteName`.
Only `resolvePackage` has the code to resolve package "exports" and it calls `resolveModulePath` as a fallback.

Changelog: [Experimental] When enabled, the `"exports"` field is now considered for Haste packages (which could include local monorepo packages)

Pull Request resolved: #1136

Test Plan: I've added a failing test which passed as the fix got implemented.

Reviewed By: huntie

Differential Revision: D51346769

Pulled By: robhogan

fbshipit-source-id: 8a003d5b147b73d344365db7cff8187ff946013d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants