huozhi / bunchee

Zero config bundler for ECMAScript and TypeScript packages
https://npmjs.com/bunchee
916 stars 29 forks source link

Deduplicate code shared between multiple exports #265

Closed thgh closed 11 months ago

thgh commented 11 months ago

It would be cool if a package that contains a programmatic API, cli and server would share the same code. Currently each export contains the complete build, but I wish the cli and server would refer to the programmatic API.

I added "." as external, but it breaks:

 » npx bunchee --target es2022 --external .
 ⨯ RollupError: "/app/node_modules/prompts/lib/index.js" is imported as an external by "/app/node_modules/prompts/lib/index.js?commonjs-external", but is already an existing non-external module id.
    at error (/app/node_modules/rollup/dist/shared/rollup.js:353:30)
    at ModuleLoader.fetchResolvedDependency (/app/node_modules/rollup/dist/shared/rollup.js:25897:24)
    at /app/node_modules/rollup/dist/shared/rollup.js:25907:189
    at async Promise.all (index 0)
    at async ModuleLoader.fetchStaticDependencies (/app/node_modules/rollup/dist/shared/rollup.js:25907:34)
    at async Promise.all (index 0)
    at async ModuleLoader.fetchModuleDependencies (/app/node_modules/rollup/dist/shared/rollup.js:25880:9)
    at async ModuleLoader.fetchModule (/app/node_modules/rollup/dist/shared/rollup.js:25868:13)
    at async Promise.all (index 2)
    at async ModuleLoader.fetchStaticDependencies (/app/node_modules/rollup/dist/shared/rollup.js:25907:34)

Skipping the prompts library results in everything being external and even an import containing an absolute path.

Also tried to update imports like . to ./index, but that doesn't work in Node 20.

Not sure if explain clearly so here is an example:

  "exports": {
    ".": {
      "import": "./index.mjs",
      "require": "./index.cjs"
    },
    "./server": {
      "import": "./server.mjs",
      "require": "./server.cjs"
    },
    "./bin": {
      "import": "./bin.mjs",
      "require": "./bin.cjs"
    }
  },
// src/server.ts
import { createServer } from '.'
const port = parseInt(process.env.PORT || '80')
createServer(port)
huozhi commented 11 months ago

Thanks for reporting, this is an interesting case. If you're importing the itself it will be marked as external by default, not sure if can be used as a workaround. Another way is to import from "./index" if you want to duplicate the that function into both bundle.

Looking into other ways to fix it

thgh commented 11 months ago

^ I put together the minimal repro code.

The ./index option results in invalid esm imports (and also invalid cjs imports but not sure why ./index doesn't work in node)

huozhi commented 11 months ago

Does this solve your problem?

// src/server.js
import { createServer } from 'entry-as-dependency'

createServer()
huozhi commented 11 months ago

And the entrypoints need to be placed in src/ folder.

If you're having ./src/server.js and ./src/index.js, using ./index should work (I got a repro locally and it works well for me).

// src/server.js
import { createServer } from './index'

createServer()
huozhi commented 11 months ago

cc @SukkaW if you know this is related to the file resolving in rollup-plugin-swc3

thgh commented 11 months ago

Does this solve your problem?

That makes Typescript not happy

Screenshot 2023-09-25 at 15 25 51

Updated the integration test and running pnpm test -- -t entry-as, but it seems i'm doing something wrong with --external?

thgh commented 11 months ago

Sorry for spamming github actions 😛

I had a typo from 'entry-as-dependency' is indeed working and just needs a .d.ts to declare the module

thgh commented 11 months ago

Hi, I tried some more cases and I think the core issue is that local import specifiers like ./index are not transpiled to a valid import specifier when marked as external like --external=./index.

- Actual output
- import { createServer } from './index'
+ Expected mjs
+ import { createServer } from './index.mjs'
+ Expected cjs
+ import { createServer } from './index.cjs'

Tested like this:

  "bin": {
    "mypkg": "dist/bin.mjs"
  },
[main●] » bunchee --target es2022 --external ./index
[main●] » npx .
node:internal/errors:496
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/.../dist/index' imported from /.../dist/bin.mjs
    at new NodeError (node:internal/errors:405:5)
    at finalizeResolution (node:internal/modules/esm/resolve:224:11)
    at moduleResolve (node:internal/modules/esm/resolve:836:10)
    at defaultResolve (node:internal/modules/esm/resolve:1034:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:375:12)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:344:25)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:220:38)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:85:39)
    at link (node:internal/modules/esm/module_job:84:36) {
  url: URL {},
  code: 'ERR_MODULE_NOT_FOUND'
}

Node.js v20.7.0

Same result in Node 16 / 18 / 20

huozhi commented 11 months ago

Marking "./index" as external is not the safe way to go, imagining they're in very different folders, ./dist/a/b/server and ./dist/c/d/index, then when you're using import mod from "./index" or "." from ./dist/a/b/server it would look for index file and module in ./dist/a/b/server.

The right way is to still import the module with the module name, such as import mod from "entry-as-dependency" if you don't want to bundle it as duplicate.

thgh commented 11 months ago

I agree that marking internal code as external is not the way to go.

Do you think bunchee could group related entries together and build them like this?

// rollup build options
  input: {
    entry1: 'src/entry1.mjs',
    entry2: 'src/entry2.mjs',
  },

That allows Rollup to automatically dedup shared parts of the code.

thgh commented 11 months ago

See the last example in the REPL: https://rollupjs.org/repl/

If multiple entries share the same dependencies, they are deduplicated automatically 🙂

huozhi commented 11 months ago

Let's say you have a library foo with two exports "." and "./server" and "./server" is relying on ".". If you want to share "." as the common dependency between them , then the module foo itself is already in the external. You just need to write import mod from "foo" in "./server", it will be treated as external. This is the way that we think from the perspective of import the output module, instead of thinking in the way of import the entry file and let bundle do the magic. It will output the import mod from "foo" in your dist file of "./server" export to fulfill the purpose of sharing dependencies. swr is using this kinda of approach now.

If you want to duplicate them on both side, you can just write import mod from "./index".

I think you're trying to go with the first one, but based on the submitted test case I see it as already supported. Feel free to post a link of repo that you're having trouble with, I'd love to take a look and solve the specific issue there

thgh commented 11 months ago

Thank you for the support! import mod from "foo" works ok.

I'm not excited having to adjust my code to fit the bundler and I had hoped for a zero-config solution. That's all 😉

huozhi commented 11 months ago

Thanks for the feedback, this is definitely an interesting case to support. import "." case is still being investigated on upstream module side, Once this case is fixed in the plugin then we can patch the support

SukkaW commented 11 months ago

@huozhi Would you like to provide the rollup configuration that bunchee will generate with the original export map?

  "exports": {
    ".": {
      "import": "./index.mjs",
      "require": "./index.cjs"
    },
    "./server": {
      "import": "./server.mjs",
      "require": "./server.cjs"
    },
    "./bin": {
      "import": "./bin.mjs",
      "require": "./bin.cjs"
    }
  },

rollup-plugin-swc3 is a low-level rollup plugin, without the low-level rollup information I can't create a minimum test case to reproduce the issue.

image
huozhi commented 11 months ago

@SukkaW sorry for the flase alarm, I did few tests locally looks like it works well. Even with bunchee, js code exports works well. It's more like for bunchee users I should provide a way to mark an export as external easily with TypeScript. Thanks!

huozhi commented 11 months ago

Going to close this as it's already supported

huozhi commented 8 months ago

@thgh This improvement is supported in 4.2.0, you can directly import the source code, for instance import ./server export into ./index like you did. And it will be treated as external for index's bundle