rollup / plugins

🍣 The one-stop shop for official Rollup plugins
MIT License
3.57k stars 567 forks source link

[@rollup/plugin-esm-shim] Shim inserted incorrectly if source file has `import` in comment #1709

Open EvanHahn opened 2 months ago

EvanHahn commented 2 months ago

Expected Behavior

The shim should be inserted in the right place, even if the source file contains static imports in comments.

For example, the shim should be inserted right after the import in the file below:

import foo from "./other_module.js";

// Triggers the shim
require();

/*
import BAD from "./bad";
*/

Actual Behavior

The shim is inserted in a strange place, causing syntax errors.

Additional Information

I believe this bug happens because the plugin tries to insert the shim after the last import, but incorrectly detects them when they're inside comments.

imports are detected with the following regular expression:

https://github.com/rollup/plugins/blob/9e7f576f33e26d65e9f2221d248a2e000923e03f/packages/esm-shim/src/constants.ts#L13-L14

This expression is unaware of comments.

I would probably solve this by parsing the AST rather than using a regular expression. Let me know if you'd like me to make that pull request.

Edgar-P-yan commented 1 month ago

Hi! I hit the same bug while was working on my library that needs to be compiled to both esm and cjs modules (https://github.com/Edgar-P-yan/node-systray-v2/). The shims are inserted in the middle of other import statements causing syntax errors. But i didn't have any commented import keywords. So probably the ESMStaticImportRegex has other bugs in it too. The version i'm using is the latest at this moment (v0.1.6). Here is the result:

import * as child from 'child_process';
import * as readline from 'readline';
import * as path from 'path';
import * as os from 'os';
import * // <-- The import statement is cut in the middle
// -- Shims --
import cjsUrl from 'node:url';
import cjsPath from 'node:path';
import cjsModule from 'node:module';
const __filename = cjsUrl.fileURLToPath(import.meta.url);
const __dirname = cjsPath.dirname(__filename);
const require = cjsModule.createRequire(import.meta.url);
 as fs from 'fs-extra'; // <-- The unfinished import continues after the inserted shims
import { EventEmitter } from 'events';
import xdebug from 'debug';

So i wanted to share a simple and temporary solution to this, a slightly modified version of this plugin. This will also not work for commented import statements, but at least the regex is simpler, so you can adjust it. See the comments for more info about patches:

/**
 * An alternative to @rollup/plugin-esm-shim (https://github.com/rollup/plugins/tree/master/packages/esm-shim).
 *
 * The original ESM shim plugin has a bug: it inserts the shims
 * in wrong places causing syntax errors. This slightly modified
 * version of it is a very simple solution, which surely will not work
 * in every case, but at list does not brake specifically for my code.
 *
 * Whats different from the original?
 * The regex used to find import statements is changed to a more
 * simple one /^import\s.*';$/gm and the whole logic is a lot simpler,
 * so you can adjust the regex to make it work with your code, if it suddenly doesn't.
 */
function esmShimCustom() {
  const ESMShim = `
// -- Shims --
import cjsUrl from 'node:url';
import cjsPath from 'node:path';
import cjsModule from 'node:module';
const __filename = cjsUrl.fileURLToPath(import.meta.url);
const __dirname = cjsPath.dirname(__filename);
const require = cjsModule.createRequire(import.meta.url);
// -- End Shims --
`;
  const CJSyntaxRegex = /__filename|__dirname|require\(|require\.resolve\(/;

  return {
    name: 'esm-shim-custom',

    renderChunk(/** @type {string} */ code, _chunk, opts) {
      if (opts.format === 'es') {
        if (code.includes(ESMShim) || !CJSyntaxRegex.test(code)) {
          return null;
        }

        let endIndexOfLastImport = -1;

        // Find the last import statement and its ending index
        for (let match of code.matchAll(/^import\s.*';$/gm)) {
          if (match.length === 0 || typeof match.index !== 'number') {
            continue;
          }

          endIndexOfLastImport = match.index + match[0].length;
        }

        const s = new MagicString(code);
        s.appendRight(endIndexOfLastImport, ESMShim);

        const sourceMap = s.generateMap({
          includeContent: true,
        });

        /** @type {string[] | undefined} */
        let sourcesContent;
        if (Array.isArray(sourceMap.sourcesContent)) {
          sourcesContent = [];
          for (let i = 0; i < sourceMap.sourcesContent.length; i++) {
            const content = sourceMap.sourcesContent[i];
            if (typeof content === 'string') {
              sourcesContent.push(content);
            }
          }
        }

        return {
          code: s.toString(),
          map: {
            ...sourceMap,
            sourcesContent,
          },
        };
      }

      return null;
    },
  };
}

And the usage is straight-forward, just copy the function esmShimCustom provided above into your rollup.config.js, and add it to output.plugins:

const options = {
  output: [
    {
      banner,
      name: 'node-systray-v2',
      exports: 'named',
      sourcemap: true,
      file: './dist/index.mjs',
      format: 'esm',
      plugins: [esmShimCustom()], // <-- insert it right here
    },
    ...
  ],
  ...
}