fix: try to find shared package.json

[nicotu01]

Fix #152

[irwinarruda]

This solution is working for me!
For some reason, referencing a local build caused other problems, so I manually changed my project's node_modules with this code, and it worked!
Since I'm not a maintainer, I won't be doing any code reviews, but here are some things I found.

• Instead of trying to find a package.json until the computer's root dir, we might want to stop at the node_modules file, preventing a wrong package.json and a false positive.

while (path.parse(currentDir).base !== 'node_modules' && currentDir !== path.parse(currentDir).root)

• We also might want to use require instead of JSON.parse(readFileSync()) since it was the option used before this change and it provides caching.
• Browserify packages like buffer, util, and process find the wrong package.json. I think it's because of the first require.resolve. But I also don't know if I should be sharing those.

[nicotu01]

Hi @irwinarruda
Thanks for the review.
I will take your first remark into account.
Concerning the second, the use of require to read the package.json is precisely the subject of this ticket. If the package.json is not exported, then it is not possible to import it like this. Hence the use of readFile.

Finally, for the third point. Do you use module federation in node contexte ?

[irwinarruda]

The problem we had was because removePathFromNpmPackage(key) only returned the name of the package. The end result would be something like require("packageName/package.json"), causing an error.

In your implementation, the potentialPackageJsonPath is an absolute path, so require should be fine (eg require("/Users/.../packageName/package.json")). However, I'm not that confident to say require is much better than JSON.parse in this case.

For some reason, I had those Browserify packages for emulating Node Apis in the browser. I don't think I even use them. Either way, for those libs, the package.json returned by findPackageJson is the project one and not from the node_modules/packageName.

[irwinarruda]

I also think #131 will affect this PR if it is merged.

[gioboa]

@irwinarruda It looks like you've carefully debugged the change, thanks

I also think #131 will affect this PR if it is merged.

This PR is stale, we can merge this one an than try to complete that one too.

[nicotu01]

Hi @irwinarruda
I understand your remarks and take them in a new commit, Thanks.
I don't understand to why browserify doesn't work

[gioboa]

I like it @nicotu01 👍
A good error message is always valuable

[gioboa]

it looks good to me.
@irwinarruda Can you double check this changes please?

[irwinarruda]

It's also working for me. Thank you!
Edit: ⚠️ There is a problem with firebase.

I don't understand to why browserify doesn't work

I'm sure it's because of the require.resolve(moduleName) function. Since Vite runs in a Node environment, it already has those native packages.

console.log("browserify", require.resolve("browserify"));
console.log("process", require.resolve("process"));
console.log("buffer", require.resolve("buffer"));
// browserify /Users/irwinarruda/Documents/project/node_modules/.pnpm/[email protected]/node_modules/browserify/index.js
// process process
// buffer buffer

The good thing now is that the code throws an error instead of getting the wrong package.json.

Error: Unable to find package.json for the module "buffer"

I think if we try to fit those packages into this implementation, we'll have to change everything... In my case, those were used by Webpack and should've been in devDependencies.

[irwinarruda]

After I sent the previous message, I tested the code in another MF project. The firebase package is now throwing an error.

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main defined in /Users/irwinarruda/Project/node_modules/firebase/package.json

Firebase does not have a main entry file. From what I understand, it's only meant to be used by its exports, such as firebase/app and firebase/auth, but it only has one package.json. I'm not sure if sharing Firebase was working properly before this change since I'm not that familiar with this code base.

There is a const usedShared = { variable in the remoteEntry.js build. Even when I pass firebase as a shared dependency, it does not appear in the usedShared variable.

[nicotu01]

Yes, there's something wrong.

After some investigations on webpack federation plugin.

• we cannot share firebase because there is no default import.
• we can share firebase/app : the version will be firebase package version
• firebase/app export points on @firebase/app. We can also share @firebase/app : the version will be @firebase/app package version which is different from firebase/app

I think the solution is to find package.json version from something that can be imported (resolved). But the function removePathFromNpmPackage removes the path :

console.log(removePathFromNpmPackage('firebase/app'));
// firebase <-- which is not importable. we want to keep `firebase/app`
console.log(removePathFromNpmPackage('@firebase/app'));
// @firebase/app

So, can we remove the usage of the function removePathFromNpmPackage ?

[irwinarruda]

I couldn't think of any other packages with the same behavior. @mui/material is also strange when shared, but not quite the same as firebase. In my opinion, it's appropriate to throw an error when trying to share firebase. Maybe we could make a better error message?

[nicotu01]

it's appropriate to throw an error when trying to share firebase

Yes, I think too.
But actually, we can't share firebase/app because of removePathFromNpmPackage.

I think removePathFromNpmPackage is not suitable. Can we remove this function ?

This function is no more useful since we find the package.json after resolve

[gioboa]

I think removePathFromNpmPackage is not suitable. Can we remove this function ?

In that file is used only there, so yes.

[zhangHongEn]

1. Please test the Nx multi-package scenario where node_modules is not located under the current project.
2. removePathFromNpmPackage cannot be removed, as it is used for these two cases:
   shared: ['react/', 'react/xxxxx.js'].

[zhangHongEn]

Make sure the tests pass, and you’re welcome to merge.

[nicotu01]

Hi @zhangHongEn
I clean the shared item name.

So, You can share react and react/

You can also share react/jsx-runtime because it is exported by react.

But you can't share react/jsx-runtime.js because it is not exported by react.

Here is the exports from react package:

// extract of package.son from react
{
  "name": "react",
  "exports": {
    ".": {
      "react-server": "./react.shared-subset.js",
      "default": "./index.js"
    },
    "./package.json": "./package.json",
    "./jsx-runtime": "./jsx-runtime.js",
    "./jsx-dev-runtime": "./jsx-dev-runtime.js"
  },
 

[irwinarruda]

For my use case, it's all working fine... However, there is an error using react/:

    federation({
      name: 'host',
      manifest: true,
      filename: 'remoteEntry.js',
      shared: {
        'react/': { singleton: true },
        'react-dom': { singleton: true },
      },
    }),

[module-federation-manifest] Cannot read properties of undefined (reading 'version')

I believe it's an error in the pluginMFManifest.ts file line 266.

[irwinarruda]

This issue is starting to get more complex than I've imagined...

I don't know if it's the best one, but the easiest solution is to keep removePathFromNpmPackage and change the getNormalizeShareItem to include every possibility.

function getNormalizeShareItem(key) {
  var options = getNormalizeModuleFederationOptions();
  var shareItem =
    options.shared[cleanShareItem(key)] ??
    options.shared[removePathFromNpmPackage(key)] ??
    options.shared[removePathFromNpmPackage(key) + '/'];
  return shareItem;
}

[gioboa]

@irwinarruda thanks for your commitment, I think we can ship a solution and then iterate

[nicotu01]

@irwinarruda I push a new commit for your usecase.
Is it ok for you ?

[irwinarruda]

Hi @nicotu01, I've tested it and there still is a problem with the package/ way of sharing modules. It does not error, but the end result is not right.

@gioboa, correct me if I'm wrong, but when we share something like:

      shared: {
        'react/': { singleton: true },
        'firebase/': { singleton: true },
      },

The goal is to automatically share every submodule if it is used in the app.

Let's say I'm using react/jsx-runtime. If I share react/, I expect to see a build chunk with the jsx-runtime code. When I tested your implementation with this example the chunk was not created (you can check the remoteEntry.js file for confirmation). It only works if you explicitly pass the module name and don't rely on the trailing slash for it.

This implementation already solves the issue we started with, but it's sometimes breaking default behavior.

[gioboa]

Yep we need to respect the official behaviour