Package Exports not working

Bekaxp commented 1 year ago

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.

You can check out this repro and try it out => https://github.com/Bekaxp/native-exports-debug
Also here is a video if it helps: https://github.com/facebook/metro/assets/13329079/3ef10456-f6e5-45aa-8d0a-f16209e5bccd

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 commented 1 year ago

Reproducible example

Bekaxp commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Bekaxp commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/facebook/metro/compare/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 commented 1 year ago

Well, that wasn't too difficult: https://github.com/facebook/metro/pull/1136. Please take it for a spin to see if it fixes your issue too 🙂

Bekaxp commented 1 year ago

@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 commented 1 year ago

leave it open until your PR is merged in?

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

facebook / metro

Package Exports not working #1128