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

Add assetExtname replacement in entryFileNames #4077

Merged
merged 5 commits into from May 15, 2021

Conversation

BPScott
Copy link
Contributor

@BPScott BPScott commented May 12, 2021

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers: #3107 #3116

Description

When using preserveModules by default rollup adds the original extension of non-js/ts files to the generated filename. E.g. foo.txt compiles to foo.txt.js (so that it doesn't clash with foo.js). This behaviour was added in #3116. This is great, however it is a little tricky to emulate when customising the output filename with entryFileNames.

If I wanted the same-as-default behaviour but outputting .mjs extensions I could pass a function to entryFilenames but it is pretty dang clunky:

{
  // ...
  preserveModules: true,
  entryFilenames: (chunkInfo) => {
    const NonAssetExtensions = ['.js', '.jsx', '.ts', '.tsx'];    
    const isAssetfile = !NonAssetExtensions.some((nonAssetExt) =>
      (chunkInfo.facadeModuleId || '').endsWith(nonAssetExt),
    );

    return `[name]${isAssetfile ? '[extname]' : ''}.mjs`;
  };

Instead I would like to propose adding an assetExtname replacement, that returns the extension prefixed with a . (same as extname) if the extension is not one of the "non asset extensions".

With this change, having rollup's default behaviour but outputting .mjs filenames can be done by setting entryFilenames to '[name][assetExtname].mjs'.

This only has an effect when preserveModules is enaled.

This allows consumers who want to customise the entry file name to
easily implement the "only add an extension for non js/ts files" logic
that rollup does if you don't specify entryFileNames.
@github-actions
Copy link

github-actions bot commented May 12, 2021

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install BPScott/rollup#asset-extname

or load it into the REPL:
https://rollupjs.org/repl/?pr=4077

@BPScott
Copy link
Contributor Author

BPScott commented May 12, 2021

Question: I couldn't see any prior art for how to name multi-word replacements so I went with camel case for assetExtname. Is that acceptable?

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, makes sense to me. Bonus points for updating the documentation. Will need to wait for GitHub Actions to work properly again before merging, though.

@codecov
Copy link

codecov bot commented May 14, 2021

Codecov Report

Merging #4077 (9d53b7c) into master (b460516) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4077   +/-   ##
=======================================
  Coverage   97.49%   97.49%           
=======================================
  Files         193      193           
  Lines        6824     6824           
  Branches     2006     2005    -1     
=======================================
  Hits         6653     6653           
  Misses         84       84           
  Partials       87       87           
Impacted Files Coverage Δ
src/Chunk.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b460516...9d53b7c. Read the comment docs.

@BPScott
Copy link
Contributor Author

BPScott commented May 14, 2021

Pushed a change to fix the linting error

@lukastaegert lukastaegert merged commit 645cc5c into rollup:master May 15, 2021
@BPScott BPScott deleted the asset-extname branch May 17, 2021 17:20
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

Successfully merging this pull request may close these issues.

None yet

2 participants