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

When preserveModules is true, virtual files should use output.entryFileNames when determining their filename #4269

Closed
BPScott opened this issue Nov 11, 2021 · 1 comment · Fixed by #4270

Comments

@BPScott
Copy link
Contributor

BPScott commented Nov 11, 2021

Rollup Version

2.59.0

Operating System (or Browser)

macos

Node Version (if applicable)

Not relevant to rollup output. But a version with support for .mjs files is needed to showcase the bug

Link To Reproduction

https://replit.com/@BPScott/rollup-plugin-babel-repl-helpers-bad-filename

Expected Behaviour

Originally raised in rollup/plugins#1031, but I think the issue might be better solved at the rollup-level rather than the plugin-level.

When output.preserveModules:true is set and there are virtual files, rollup should use output.entryFileNames to generate the filename of created virtual files.

When given the following output options, virtual files should be output with a .mjs extension, regardless of their original file extension:

 output: {
    format: 'esm',
    dir: 'output',
    entryFileNames: '[name][assetExtname].mjs',
    preserveModules: true,
  },

I.e. in the given example that uses plugin-babel - ./_virtual/_rollupPluginBabelHelpers.js should be named ./_virtual/_rollupPluginBabelHelpers.mjs.

Actual Behaviour

When output.preserveModules:true and output.entryFilenames is set and there are virtual files, rollup does not use entryFileNames to generate the filename of created virtual files.

In the above demo the _rollupPluginBabelHelpers virtual file is output with the filename ./_virtual/_rollupPluginBabelHelpers.js.

This then breaks running compiled output in node as node assumes that contents of ./_virtual/_rollupPluginBabelHelpers.js is commonjs, but it is actually esm.

Bonus thoughts

I believe the fix for this lives within Chunk.ts's generateIdPreserveModules . I've tried moving the definition of fileName above the isAbsolute() if block, and changing setting the virtual path block to be "path = _virtual/${fileName};". This seems to give me the filename I'm after, but I get lots of test failures that I can't seem to resolve by updating expected sample output, so clearly I'm missing a trick somewhere.

@BPScott
Copy link
Contributor Author

BPScott commented Nov 14, 2021

It looks like fixing this would also solve rollup/plugins#1037 which relates to virtual files getting any file extension before.

In the past there has been a few bugfixes at the plugin level where plugins had to add a .js extension to virtual files -
rollup/rollup-plugin-commonjs#373 and rollup/rollup-plugin-babel#296, which probably could also have been avoided by this proposed fix at the rollup level.


Doing the below in the else block in generateIdPreserveModules seems to be looking promising.
Hopefully I'll be able to raise a PR in the next few days.

const extension = extname(sanitizedId);
const fileName = renderNamePattern(
    typeof pattern === 'function' ? pattern(this.getChunkInfo()) : pattern,
    'output.entryFileNames',
    {
        assetExtname: () => (NON_ASSET_EXTENSIONS.includes(extension) ? '' : extension),
        ext: () => extension.substr(1),
        extname: () => extension,
        format: () => options.format as string,
        name: () => basename(sanitizedId, extension)
    }
);
path = `_virtual/${fileName}`;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant