Enabling `bundle: false` does not add import extensions in ESM files

lachlancollins commented 1 year ago

I've noticed this bug after merging the following PR: https://github.com/TanStack/query/pull/5597

When bundle: true is set, index.js (ESM) file looks like this:

import "./chunk-55J6XMHW.js";
import {
  QueriesObserver
} from "./chunk-FVYPMHO2.js";
import {
  QueryClient
} from "./chunk-6F4EPMPD.js";
import {
  QueryCache
} from "./chunk-5EQKN4WB.js";
import "./chunk-CSKZ3U2N.js";
...

When bundle: false is set, index.js (ESM) file looks like this:

import { CancelledError } from "./retryer";
import { QueryCache } from "./queryCache";
import { QueryClient } from "./queryClient";
import { QueryObserver } from "./queryObserver";
import { QueriesObserver } from "./queriesObserver";
import { InfiniteQueryObserver } from "./infiniteQueryObserver";
...

I expected that tsup would also add the required .mjs (or .js when "type": "module" is set) to relative imports within the project, like rollup did. This causes import errors when imported in ESM projects (e.g. Vite). For now, I've added https://github.com/favware/esbuild-plugin-file-path-extensions, but this seems to force override some tsup settings.

Upvote & Fund

We're using Polar.sh so you can upvote and help fund this issue.
We receive the funding once the issue is completed & confirmed by you.
Thank you in advance for helping prioritize & fund our backlog.

sakulstra commented 1 year ago

Have been debugging quite a while till i found this - would it make sense to have this built in? I assume otherwise most build will effectively be broken when using cjs + esm output?

mcaskill commented 1 year ago

While trying tsup, I stumbled upon a similar issue with bundle: false and bundling CJS and ESM.

If my package.json is set to ESM ("type": "module"):

the ESM files will use .js and their imports will point to .js (the extension I'm using in my TS files).
the CJS files will use .cjs but their requires will point to .js.

If my package.json is set to CJS, I get the opposite issue where the ESM files are importing .js instead of .mjs.

If I'm using a single entry-point, then there's no issue because there's no importing.

For context, I'm aiming for Node 16+, I'm building a utilities/helpers package that has no dependencies and could even be used from the browser via ESM, and this is my tsup configuration:

{
    "bundle": false,
    "clean": true,
    "dts": true,
    "entry": [
        "src/**/*.ts"
    ],
    "format": [
        "cjs",
        "esm"
    ],
    "minify": false,
    "sourcemap": true,
    "splitting": false
}

I'm able to patch the CJS or MJS files with the following:

find dist -type f \( -name '*.cts' -o -name '*.cjs' -o -name '*.cjs.map' \) -exec sed -i 's/\.js\b/.cjs/g' {} +

smeijer commented 1 year ago

Did something similar, but via a plugin:

export default defineConfig({
  entry,
  clean: true,
  dts: true,
  format: ['cjs', 'esm'],
  plugins: [
    {
      name: 'fix-cjs',
      renderChunk(_, chunk) {
        if (this.format === 'cjs') {
          // replace `from '...js'` with `from '...cjs'` for cjs imports & exports
          const code = chunk.code.replace(/from ['"](.*)\.js['"]/g, "from '$1.cjs'");
          return { code };
        }
      },
    },
  ],
});

capJavert commented 6 months ago

Pardon my french but it is bulls*** that this does not work.

edit: my apologies for rough language either way

JesusTheHun commented 6 months ago

@capJavert I'm french and I don't pardon you. This is OSS and you are posting on a public issue tracker. This is not a place to rant.

Not happy with it ? Submit a PR.

If you don't want to do that or can't for whatever reason, be polite or be quiet.

alexreardon commented 5 months ago

Hi all, I ran into what I think is a related issue today:

package.json → "module": true
generating esm and cjs outputs
using bundle:false
my cjs outputs have require('./add').
Tue to the "module": true the require('./add') is looking up the add.js (esm) version of the output and not the .cjs variant add.cjs. A fix is that the require call should always look up the .cjs variant require('./add.cjs')

For now I'll look into doing a find / replace for require in the .cjs files so they always look up the .cjs variants

alexreardon commented 5 months ago

My config, and also my current work around:

import { defineConfig } from 'tsup';

export default defineConfig({
  entry: ['src/*'],
  bundle: false,
  clean: true,
  format: ['cjs', 'esm'],
  dts: true,
  tsconfig: './tsconfig.json',
  plugins: [
    {
      // https://github.com/egoist/tsup/issues/953#issuecomment-2132576167
      // ensuring that all local requires in `.cjs` files import from `.cjs` files.
      // require('./path') → require('./path.cjs') in `.cjs` files
      name: 'fix-cjs-require',
      renderChunk(_, { code }) {
        if (this.format === 'cjs') {
          const regex = /require\("(?<import>\.\/.+)"\)/g;
          // TODO: should do nothing if file already ends in .cjs
          // TODO: could be more resilient for `"` vs `'` imports
          return { code: code.replace(regex, "require('$<import>.cjs')") };
        }
      },
    },
  ],
});

Andarist commented 5 months ago

In addition to that, explicit extensions in source files break output:

// input: src/bind-all.ts
import type { Binding, InferEventType, Listener, UnbindFn } from './types.js';
import { bind } from './bind.js';

// output: dist/bind-all.cjs
var import_bind = require("./bind.js");

// output: dist/bind-all.d.cts
import { InferEventType, Listener, UnbindFn } from './types.cjs';

// output: dist/bind-all.d.ts
import { InferEventType, Listener, UnbindFn } from './types.js';

// output: dist/bind-all.js
import { bind } from "./bind.js";

The extensions in output declaration files are OK but a .cjs file tries to require a .js file in this output.

ST-DDT commented 2 months ago

We are also affected by this.

Which file do I need to look at if I want to submit a PR to fix this?

JoshuaKGoldberg commented 2 months ago

The renderChunk options mentioned earlier aren't working for me 😕. The chunk text they're getting doesn't seem to have the import or require calls in it.

Instead, I ended up adding a postbuild script that points to a file containing:

import { globIterate } from "glob";
import fs from "node:fs/promises";

for await (const entry of globIterate("lib/cjs/**/*.cjs")) {
    await fs.writeFile(
        entry,
        (await fs.readFile(entry))
            .toString()
            .replaceAll(/require\("(.+)\.js"\);/g, 'require("$1.cjs");'),
    );
}

🤷.

glitch452 commented 2 months ago

My config, and also my current work around:

import { defineConfig } from 'tsup';

export default defineConfig({
  entry: ['src/*'],
  bundle: false,
  clean: true,
  format: ['cjs', 'esm'],
  dts: true,
  tsconfig: './tsconfig.json',
  plugins: [
    {
      // https://github.com/egoist/tsup/issues/953#issuecomment-2132576167
      // ensuring that all local requires in `.cjs` files import from `.cjs` files.
      // require('./path') → require('./path.cjs') in `.cjs` files
      name: 'fix-cjs-require',
      renderChunk(_, { code }) {
        if (this.format === 'cjs') {
          const regex = /require\("(?<import>\.\/.+)"\)/g;
          // TODO: should do nothing if file already ends in .cjs
          // TODO: could be more resilient for `"` vs `'` imports
          return { code: code.replace(regex, "require('$<import>.cjs')") };
        }
      },
    },
  ],
});

Thanks for this!

I enhanced it a little with:

Support both ESM and CJS source files
Use either ' or " quotes
Import files in a parent directory with ../
Only matching on .js imports.

plugins: [
  {
    // https://github.com/egoist/tsup/issues/953#issuecomment-2294998890
    // ensuring that all local requires/imports in `.cjs` files import from `.cjs` files.
    // require('./path') → require('./path.cjs') in `.cjs` files
    // require('../path') → require('../path.cjs') in `.cjs` files
    // from './path' → from './path.cjs' in `.cjs` files
    // from '../path' → from '../path.cjs' in `.cjs` files
    name: 'fix-cjs-imports',
    renderChunk(code) {
      if (this.format === 'cjs') {
        const regexCjs = /require\((?<quote>['"])(?<import>\.[^'"]+)\.js['"]\)/g;
        const regexEsm = /from(?<space>[\s]*)(?<quote>['"])(?<import>\.[^'"]+)\.js['"]/g;
        return {
          code: code
            .replace(regexCjs, 'require($<quote>$<import>.cjs$<quote>)')
            .replace(regexEsm, 'from$<space>$<quote>$<import>.cjs$<quote>'),
        };
      }
    },
  },
],

altano commented 2 months ago

Once you've solved the problem with having to add extensions to all your source files (ugh), another way to deal with having a hybrid CJS/ESM package is to output the two to separate directories, both using .js as the extension. This avoids needing a post-processing step to patch ".js" to ".cjs"

...

~~then do something like this in your package.json:~~

...

~~You'd probably want to generate types with separate tsconfigs for each build too.~~

~~I think that's a little cleaner?~~

EDIT:

^^^^ That doesn't work because the consuming package won't care if the imported package is using a "require" conditional export, because the package is marked type=module and the extension is js, so the CJS code will erroneously be treated as ESM code:

Error [ERR_REQUIRE_ESM]: require() of ES Module /.../node_modules/@altano/repository-tools/dist/cjs/findRootSync.js from /project/lib/rules/valid-repository-directory.js not supported.

findRootSync.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.

Instead either rename findRootSync.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in /project/node_modules/.pnpm/@altano+repository-tools@0.1.0/node_modules/@altano/repository-tools/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).

Lonli-Lokli commented 1 month ago

I have a similar error, with CJS/ESM generation. Error happens with ESM source.ts import countBy from "lodash/countBy";

index.js (ESM) import countBy from "lodash/countBy";

When executing this test.mjs import { MyClass} from 'my-package' I am getting this $ node source.mjs Error [ERR_MODULE_NOT_FOUND]: Cannot find module '..node_modules#lodash#countBy' imported from 'xxx'. Did you mean to import 'lodash/countBy.js'?

diego-escobedo commented 1 month ago

same issue @Lonli-Lokli . However I don't have bundle set so its defaulting to true. Also have splitting set to false, target set to node20, and format as just esm

altano commented 1 month ago

@diego-escobedo if you’re bundling it shouldn’t matter because you aren’t importing any local files, and files coming from packages don’t need extensions.

Lonli-Lokli commented 1 month ago

I finally ended up with replacing import countBy from "lodash/countBy" into import { countBy } from "lodash-es"

aymericzip commented 1 week ago

I faced similar import path issues when using tsup with bundle: false. The problems included:

Missing File Extensions: Import statements lacked explicit file extensions like .js, causing runtime errors in some environments.
Directory Imports: Importing from directories without specifying index files led to module resolution issues.
Unresolved Path Aliases: Aliases defined in tsconfig.json weren't correctly resolved in the output.

To solve this, I created esbuild-fix-imports-plugin, which combines three ESBuild plugins to fix these issues:

fixAliasPlugin: Resolves path aliases from tsconfig.json to relative paths.
fixFolderImportsPlugin: Converts directory imports to explicit paths pointing to index files.
fixExtensionsPlugin: Appends correct file extensions to relative import paths.

Usage:

Install the plugin:
```
npm install esbuild-fix-imports-plugin
```

Update your tsup.config.ts:

import { defineConfig } from 'tsup';
import { fixImportsPlugin } from 'esbuild-fix-imports-plugin';

export default defineConfig({
 // Your other configurations
 bundle: false, // Important
 esbuildPlugins: [fixImportsPlugin()],
 // ... other settings
});

This should resolve the import path issues when bundle is set to false. Let me know if this helps!

Code: https://github.com/aymericzip/esbuild-fix-imports-plugin

Package: https://www.npmjs.com/package/esbuild-fix-imports-plugin

Issue: https://github.com/egoist/tsup/issues/1240

egoist / tsup

Enabling `bundle: false` does not add import extensions in ESM files #953

Upvote & Fund