search

evanw / esbuild

An extremely fast bundler for the web
https://esbuild.github.io/
MIT License
38.16k stars 1.15k forks source link

Transitive dependencies do not work with aliased package names #3339

Open mohamedmansour opened 1 year ago

mohamedmansour commented 1 year ago

esbuild version: 0.19.2

Hello! We are trying to use aliased node_modules packages, and when doing so, it doesn't understand transitive dependencies. For example '@microsoft/fast-foundation-v3 (aliased package) has a dependency to @microsoft/fast-element (normal package).. The screenshot below showcases that it is confused by importing the non-suffix -v3 package, causing the generated code to be in a bad state and not working. Big chunk of code being bundled incorrectly and broken bundle.

❌ Incorrect and not working image

✔️ Correct and working by using a custom esbuild plugin image

The only way I can make it work today is by writing a ESBuild Resolver Plugin, which replaces @microsoft/fast-element to @microsoft/fast-element-v3. We want multiple versions of the same package, and was hoping the node module resolution respects versions, but it does not. The plugin code is available here https://github.com/mohamedmansour/web-components-perf/blob/main/infra/plugins/fluentui_esmodule_resolver_plugin.ts

Here is the minimum repro. Available in Gist as well so you can fork https://gist.github.com/mohamedmansour/9c80e1eef158eba11d1043bee37d2297, since it is an esmodule, you need to python -m http.server.

app.ts

import { customElement, html, FASTElement } from '@microsoft/fast-element-v3';
import { DesignToken } from '@microsoft/fast-foundation-v3/design-token.js';
import { setTheme } from '@fluentui/web-components-v3';
import { webLightTheme } from '@fluentui/tokens';
import "@fluentui/web-components-v3/button.js";

@customElement({
  name: 'example-app',
  template: html`
  <div>
    <fluent-button>Fluent Button</fluent-button>
  </div>`
})
export class ExampleApp extends FASTElement {}

DesignToken.registerDefaultStyleTarget();
setTheme(webLightTheme);

build.cjs

const path = require('node:path')
const fs = require('node:fs')
const esbuild = require('esbuild')

const distPath = path.join(__dirname, 'dist')

esbuild.build({
    bundle: true,
    outdir: distPath,
    entryPoints: [path.join(__dirname, 'app.ts')],
    format: 'esm',
    minify: true,
    target: 'esnext',
    sourcemap: true,
    tsconfigRaw: {
        compilerOptions: {
            module: 'esnext',
            target: 'esnext',
            experimentalDecorators: true,
            strict: true,
            useDefineForClassFields: false
        }
    },
    metafile: true
}).then(results => {
    fs.mkdirSync(distPath, { recursive: true })
    fs.writeFileSync(path.join(distPath, 'meta.json'), JSON.stringify(results.metafile, null, 2))
})

package.json

{
    "name": "example_app",
    "version": "1.0.0",
    "description": "Example App",
    "type": "module",
    "scripts": {
        "start": "node build.cjs"
    },
    "author": "",
    "license": "ISC",
    "dependencies": {
        "@fluentui/tokens": "1.0.0-alpha.2",
        "@fluentui/web-components-v3": "npm:@fluentui/web-components@3.0.0-alpha.30",
        "@microsoft/fast-element-v3": "npm:@microsoft/fast-element@2.0.0-beta.26",
        "@microsoft/fast-foundation-v3": "npm:@microsoft/fast-foundation@3.0.0-alpha.31",
        "@microsoft/fast-web-utilities-v3": "npm:@microsoft/fast-web-utilities@6.0.0"
    },
    "devDependencies": {
        "esbuild": "0.19.2"
    }
}
hyrious commented 1 year ago

The npm:pkg@version syntax in package.json does not actually mean package aliasing. This feature is just installing the specific package into some node_modules/{name}. You will potentially break the node resolution behavior because of this.

To alias packages, esbuild has an option --alias:@microsoft/fast-element=@microsoft/fast-element-v3. You can also use tsconfig's paths field or a custom resolving plugin to do so (you already did).

I'm not sure why you want users writing different import paths than the original package.

jakebailey commented 1 year ago

I don't think that's accurate; package managers gracefully handle installing multiple versions of the same libraries all the time, so they must be able to handle multiple sets of transitive dependencies without breaking things.

DanielRosenwasser commented 1 year ago

I don't want to derail too much further, but I do agree with Jake - I don't think there's anything wrong with using npm:pkg@version specifiers.

And that said, I think a more ideal path would be to lean on overrides, rather than overriding resolution in any capacity - whether it's through a plugin or through --alias or whatever.

hyrious commented 1 year ago

they must be able to handle multiple sets of transitive dependencies without breaking things

Do you mean that you believe npm should handle your case carefully that it finds out that @microsoft/fast-foundation-v3's dependency @microsoft/fast-element should be resolved to @microsoft/fast-element-v3? i.e. Your expected node_modules tree should be like this:

node_modules/
    @microsoft/
        fast-foundation-v3/
            some-file.js "import '@microsoft/fast-element'"
        fast-element/ (symlink to ./fast-element-v3/)
        fast-element-v3/
            ...

Let me explain why the folder node_module/@microsoft/fast-element exists: Because package managers cannot look into and change your source code, all they can do is analyzing the package.json file and calculate the correct node_modules tree. Since there is package.json said its dependencies including @microsoft/fast-element, to make Node.js resolution work, it must create a symlinked folder.

Ok now let's see what actually happens,

$ mkdir test && cd test
$ cat <<EOF >package.json
{
  "dependencies": {
    "@microsoft/fast-element-v3": "npm:@microsoft/fast-element@2.0.0-beta.26",
    "@microsoft/fast-foundation-v3": "npm:@microsoft/fast-foundation@3.0.0-alpha.31"
  }
}
EOF
$ npm i

The node_modules folder now looks like this:

node_modules/
    @microsoft/
        fast-foundation-v3/
        fast-element/       (?, this is NOT a symlink)
        fast-element-v3/

Why? This is because npm actually did not figure out what you want. When it saw @microsoft/fast-foundation-v3's package.json says it has a dependency of @microsoft/fast-element, it just fetches the actual @microsoft/fast-element package and put it there.

I think a more ideal path would be to lean on overrides

That's half true, now let's try adding another config to the root package.json:

{
  "dependencies": {
    "@microsoft/fast-element-v3": "npm:@microsoft/fast-element@2.0.0-beta.26",
    "@microsoft/fast-foundation-v3": "npm:@microsoft/fast-foundation@3.0.0-alpha.31"
  },
  "overrides": {
    "@microsoft/fast-element": "@microsoft/fast-element-v3"
  }
}

Run npm install again, you will find that npm never generates node_module/@microsoft/fast-element now. However, the bundling will still fail because the actual code is still importing this package:

// somewhere in the @microsoft/fast-foundation-v3
import { html, ref } from "@microsoft/fast-element";

And then esbuild/node.js will perform the Node.js resolution algorithm and finds out there's no folder named fast-element :/

If you still believe that this case should be correctly handled by the package manager (npm), then you should raise an issue there: http://github.com/npm/cli instead of here in the bundler side. One the other hand, if esbuild's resolver has any different behavior than the Node.js one then esbuild should be able to fix that.

mohamedmansour commented 1 year ago

Thank you @hyrious, I tried the overrides feature and got what you got as well. Back to your original question on "why I want this" it is because in Chromium (Browser Development), we basically have a single node_modules folder, and we host around 50+ Distinct Websites (where Chromium is the web server). Many packages are being shared with many different websites, and if we want to do a major upgrade from V1 to V2 (Breaking Change), we want to do it incrementally. The reason of using a bundler is because we can store all 50+ Website's entry point into the bundler, it will treeshake and produce chunks with code splitting. That way we will have a binary which is lighter, and each page's first contentful paint will be the quick since we have 0 network latency. Another approach is to create ESModules and host them in the browser. I used esbuild to generate an ESModule per UI Component and per Library, and that worked (with ESBuild resolver plugin to rewrite the transitive dependencies as ESModule, like import {FASTElement} from './fast-element.js', since it transformed @microsoft/fast-element to ./fast-element.js), but the challenge here now, how do you generate the TypeScript descriptor files for the ESBuild generated bundles since it includes transitive dependencies. So I guess I have to write more custom tooling.

I opened a feature request in https://github.com/microsoft/fluentui/issues/28974 to so it can make their ESModules not depend on NodeJS Modules.

Here is what I did with overrides:

X [ERROR] Could not resolve "@microsoft/fast-foundation/utilities.js"

    node_modules/@fluentui/web-components-v3/dist/esm/text-input/text-input.styles.js:2:24:     
      2 │ import { display } from '@microsoft/fast-foundation/utilities.js';
        ╵                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

like the following in package.json

    "overrides": {
        "@fluentui/web-components-v3": {
            "@microsoft/fast-element": "@microsoft/fast-element-v3",
            "@microsoft/fast-foundation": "@microsoft/fast-foundation-v3"
        },
        "@microsoft/fast-foundation-v3": {
            "@microsoft/fast-element": "@microsoft/fast-element-v3"
        }
    },

where package-lock.json did the following mutation: image image

hyrious commented 1 year ago

Ah, I see your purposes. In your first approach (use esbuild to bundle all sites with code splitting enabled), you basically need an alias option that supports to aliasing packages not only by their names, but also by their locations (i.e. aliasing the @microsoft/fast-element in @fluentui/web-components-v3 to @microsoft/fast-element-v3). You can name it "scoped alias". This is possible with the plugins API, see https://esbuild.github.io/plugins/#resolve

Let's write a plugin that uses the info above:

var config = package_json.overrides
onResolve({ filter: /^[\w@]/ }, (args) => {
  for (var key in config) {
    // if we are in e.g. 'node_modules/@microsoft/fast-foundation-v3'
    if (args.resolveDir.includes("node_modules/" + key)) {
      var map = config[key];
      for (var key2 in map) {
        // replace imports to '@microsoft/fast-element' with '@microsoft/fast-element-v3'
        if (args.path == key2 || args.path.startsWith(key2 + "/")) {
          var path = args.path.replace(key2, map[key2]);
          delete args.path;
          // `resolve()` is provided by the plugin API
          return resolve(path, args);
        }
      }
    }
  }
});
mohamedmansour commented 1 year ago

Thanks @hyrious, how does transitive imports work for that if our overrides are:

   "overrides": {
        "@fluentui/web-components-v2": {
            "@microsoft/fast-element": "@microsoft/fast-element-v2",
            "@microsoft/fast-foundation": "@microsoft/fast-foundation-v2"
        },
        "@microsoft/fast-foundation-v2": {
            "@microsoft/fast-element": "@microsoft/fast-element-v2"
        },
        "@fluentui/web-components-v3": {
            "@microsoft/fast-element": "@microsoft/fast-element-v3",
            "@microsoft/fast-foundation": "@microsoft/fast-foundation-v3"
        },
        "@microsoft/fast-foundation-v3": {
            "@microsoft/fast-element": "@microsoft/fast-element-v3"
        }
    },

The plugin resolver will get called for every single import @fluentui/web-components-v2 and @fluentui/web-components-v3 have which would be just @microsoft/fast-foundation so we wouldn't know which one to replace it with, V2 or V3.

I am going to try to convince FluentUI folks to create ESModules that do not have NodeJS imports, this is getting way to unnatural for bundlers I would think. And for now, to make it simpler, I can use alias instead of plugins

https://github.com/microsoft/fluentui/issues/28974

jakebailey commented 1 year ago

Why? This is because npm actually did not figure out what you want. When it saw @microsoft/fast-foundation-v3's package.json says it has a dependency of @microsoft/fast-element, it just fetches the actual @microsoft/fast-element package and put it there.

I feel like there's some fundamental confusion between us here; I'm saying that one can write:

  "dependencies": {
    "@microsoft/fast-foundation": "^2",
    "@microsoft/fast-foundation-v3": "npm:@microsoft/fast-foundation@3.0.0-alpha.31"
  }

And package managers will happly make this work. npm gives a tree like:

image

That is, fast-foundation-v3's imports see the "correct" fast-element, not the lower version required by fast-foundation@2:

image

pnpm makes this even more clear, creating specific directory structures to ensure that things work:

image