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

Using shims and codesplitting together has subtle issues. #958

Open
ramblingenzyme opened this issue Aug 2, 2023 · 1 comment
Open

Using shims and codesplitting together has subtle issues. #958

ramblingenzyme opened this issue Aug 2, 2023 · 1 comment

Comments

@ramblingenzyme
Copy link

ramblingenzyme commented Aug 2, 2023

All of these edge cases come into play when the shims end up in a different chunk than they're used in.

CJS shim edge cases

When outputting a ESM bundle with shims enabled:

  • __filename will never be correct as the import.meta.url will be the chunk it's exported from, not the one it's used in.
  • __dirname will be incorrect if the chunk containing the shims is in a different directory than the chunk importing the shims.
dist
├── a
│   └── index.js // <- __filename will be /dist/chunk-with-shims.js, not /dist/a/index.js
├── b
│   └── index.js // <- if the __dirname shims are imported in this chunk, it will be /dist, not /dist/b
├── c
│   └── index.js
├── chunk-with-shims.js
└── ...

Edge cases when using ESM shims

For the ESM shims, import.meta.url will never be correct if the shims are in a chunk.

dist
├── a
│   └── index.js
├── b
│   └── index.js
├── c
│   └── index.js
├── chunk-with-shims.js <- the importMetaUrl export uses __filename, which will always be dist/chunk-with-shims.js
└── ...
var _shimChunk = require('../chunk-with-shims.js');
var _fs = require('fs');
var _fs2 = _interopRequireDefault(_fs);

// This is adapted from https://nodejs.org/api/esm.html#importmetaurl
// _shimChunk.importMetaUrl will be the __filename of `../chunk-with-shims.js`, not this file
const buffer = _fs2.default.readFileSync(new URL('./data.proto', _shimChunk.importMetaUrl));

One impact of this, is that it breaks checking whether a module is main. https://exploringjs.com/nodejs-shell-scripting/ch_nodejs-path.html#detecting-if-module-is-main

import * as url from 'node:url';

if (import.meta.url.startsWith('file:')) { // (A)
  // import.meta.url will be wrong if the shim ends up in a chunk, and this
  // will never run when called from a shell.
  const modulePath = url.fileURLToPath(import.meta.url);
  if (process.argv[1] === modulePath) { // (B)
    // Main ESM module
  }
}

How to fix it

The ESM/CJS shims should be injected into each chunk that requires them, rather than using the inject feature of esbuild as it's currently done. https://github.com/egoist/tsup/blob/v7.1.0/src/esbuild/index.ts#L220

This could be done with an ESBuild plugin, which modifies the output chunks after the build, or as a tsup plugin.

This is a pretty hacky plugin I whipped up to replace the tsup shims when building ESM bundles. Having poked around the code base, (and seeing how tsup plugins work), this likely breaks sourcemaps, but they aren't turned on in our project.

import { Options } from "tsup";
import * as path from "path";
import * as fs from "fs";

// https://stackoverflow.com/a/51399781
type ArrayElement<ArrayType extends readonly unknown[]> =
    ArrayType extends readonly (infer ElementType)[] ? ElementType : never;

type EsbuildPlugin = ArrayElement<NonNullable<Options["esbuildPlugins"]>>;


const CjsShimPlugin: EsbuildPlugin = {
    name: "cjs-shim-plugin",
    setup(build) {
        build.onEnd((result) => {
            const filesNeedingShims =
                result.outputFiles?.filter(
                    (file) => file.text.match(/__dirname|__filename/) !== null
                ) || [];

            filesNeedingShims.map((file) => {
                const dirnameShim = `
                    import { fileURLToPath as fileURLToPathShimImport } from "node:url";
                    import { dirname as dirnamePathShimImport } from "node:path";
                    const __filename = fileURLToPathShimImport(import.meta.url);
                    const __dirname = dirnamePathShimImport(__filename);
                `;

                const shimBuffer = Buffer.from(dirnameShim);
                file.contents = Buffer.concat([shimBuffer, file.contents]);
            });
        });
    },
};

export default CjsShimPlugin;

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.
Fund with Polar
@ramblingenzyme
Copy link
Author

How to avoid it today

  • Don't mix ESM & CJS
  • Avoid bundling dependencies that require shims if possible, or turn off codesplitting.

Similar issue: #914, CJS bundles with code splitting enabled is likely to break the CJS version of checking if a module is main because something generates a wrapper around require and it gets put in a different chunk, so the .main property references the shim chunk, not the chunk the shim is used in.

if (require.main === module) {
  // Main CommonJS module
}

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

No branches or pull requests

1 participant