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

Incorrect resolution for nested node_modules due to usage of basedir in NodeResolvePlugin #233

Open
amoshydra opened this issue Aug 31, 2024 · 1 comment · May be fixed by #234
Open

Incorrect resolution for nested node_modules due to usage of basedir in NodeResolvePlugin #233

amoshydra opened this issue Aug 31, 2024 · 1 comment · May be fixed by #234

Comments

@amoshydra
Copy link

amoshydra commented Aug 31, 2024

  • mdx-bundler version: 10.0.3
  • node version: 18.8.0, 18.14.1, 20.12.1
  • npm version: 8.18.0, 9.5.0, 10.5.0
  • esbuild version: 0.19.12, 0.23.1
  • @esbuild-plugins/node-resolve version: 0.2.2

Relevant code or config

const result = await bundleMdx({
  file: "c:/project/index-c.mdx",
  cwd: "c:/project",
});

// c:/project/index-c.mdx

import packageC from "package-c";

<span>{packageC}</span>

I have identified that usage of basedir in NodeResolvePlugin causes the incorrect module resolution to occur for nested node_modules.

mdx-bundler/src/index.js

Lines 147 to 150 in 616c5b0

NodeResolvePlugin({
extensions: ['.js', '.ts', '.jsx', '.tsx'],
resolveOptions: {basedir: cwd},
}),

https://github.com/kentcdodds/mdx-bundler/pull/68/files#diff-bfe9874d239014961b1ae4e89875a6155667db834a410aaaa2ebe3cf89820556R149

What you did:

Attempt to bundle a component using package (package-a) with 2 different versions, imagine the node_modules structure like this

project
├── index-b.js
├── index-c.js
└── node_modules
    ├── package-a (1.0.0)
    │   ├── a.js
    │   └── package.json
    ├── package-b
    │   ├── b.js
    │   └── package.json
    └── package-c
        ├── c.js
        ├── node_modules
        │   └── package-a (2.0.0)
        │       ├── a.js
        │       └── package.json
        └── package.json

* exact version doesn't matter here, only the folder structure matter

  • index-b imports package-b imports package-a
    • package-a should resolve to node_modules/package-a/a.js
  • index-c imports package-c imports package-a
    • package-a should resolve to node_modules/package-c/node_modules/package-a/a.js

What happened:

The incorrect module is resolved.
mdx-bundler always resolve module at ${process.cwd()}/node_modules, breaking NodeJS's / ESBuild node module resolution logic.

  • index-b imports package-b imports package-a
    • package-a resolved to node_modules/package-a/a.js
  • index-c imports package-c imports package-a
    • package-a resolved to node_modules/package-a/a.js <-- ❌ incorrect resolution

Reference:

Reproduction repository:

A basic reproduce repo is created to demonstrate this point using only esbuild and @esbuild-plugins/node-resolve

https://github.com/amoshydra/repro-kentcdodds-mdx-bundler/tree/repro-issue-233

Test setup

build.mjs contains 3 setup:

  • esbuild without plugin
  • esbuild with @esbuild-plugins/node-resolve using default option
  • esbuild with @esbuild-plugins/node-resolve passing project's working directory as basedir

Observation

dist folder contain the output for each build

Problem description:

mdx-bundler should follow NodeJS module resolution, but it does not.

Consequently, as we migrate our bundler from webpack/docz to mdx-bundler, we observe many of the packages failed to run due to incorrect module resolution

Suggested solution:

For the scope of my project, the following workarounds works for me:

  • removing @esbuild-plugins/node-resolve, let esbuild perform the resolution
    (mdx-bundler cannot load md file if plugin is removed. mdx loads fine)
  • passing our own @esbuild-plugins/node-resolve without the basedir option
    const result = await bundleMdx({
      file: "c:/project/document/document.mdx",
      cwd: "c:/project",
      esbuildOptions: (options) => {
        const nodeResolvePluginIndex = options.plugins.findIndex(({ name }) => name === "node-resolve");
        if (nodeResolvePluginIndex >= 0) {
          // replace original plugin with our own without `resolveOptions.basedir` configured
          options.plugins.splice(nodeResolvePluginIndex, 1, NodeResolvePlugin({
            extensions: [".ts", ".tsx", ".js", ".jsx"],
          }); 
        }
        return options;
      },
    });

Earlier, I wanted to suggest removing basedir.
However basedir appear to be added intentionally in #68. I have yet to look into what problem #68 is trying to solve

amoshydra added a commit to amoshydra/mdx-bundler that referenced this issue Sep 1, 2024
amoshydra added a commit to amoshydra/mdx-bundler that referenced this issue Sep 1, 2024
@amoshydra
Copy link
Author

amoshydra commented Sep 1, 2024

I am unable to reproduce the issue raised in #66 / #68 for these combination of versions:

package version depended
node 18.8.0, 18.14.1, 20.12.1
npm 8.18.0, 9.5.0, 10.5.0
mdx-bundler 10.0.3
esbuild 0.19.12, 0.23.1 by mdx-bundler
@esbuild-plugins/node-resolve 0.2.2 by mdx-bundler
resolve 1.22.8 by @esbuild-plugins/node-resolve

I have thus raised #234

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.

1 participant