Include extensions in preserveModules output filenames for scriptified assets #3116
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3116 +/- ##
==========================================
+ Coverage 89.31% 89.32% +0.01%
==========================================
Files 165 165
Lines 5728 5734 +6
Branches 1738 1739 +1
==========================================
+ Hits 5116 5122 +6
Misses 379 379
Partials 233 233
Continue to review full report at Codecov.
|
src/Chunk.ts
Outdated
| const name = renderNamePattern( | ||
| options.entryFileNames || '[name].js', | ||
| options.entryFileNames || | ||
| (COMMON_JS_EXTENSIONS.includes(extension) ? '[name].js' : '[name].[ext].js'), |
There was a problem hiding this comment.
Not sure why the name is COMMON_JS... as I do not see the connection. How about JAVASCRIPT_EXTENSION or IGNORED_EXTENSIONS? Also, I would rather have the default pattern to be [name][extname].js, the reason being that otherwise, files without extension would have two subsequent . in their name.
There was a problem hiding this comment.
JAVASCRIPT_EXTENSIONS would work, just seemed a little bit inaccurate as it also includes TS extensions - i'm ok with renaming it though to whatever you see consider fitting here
Also, I would rather have the default pattern to be [name][extname].js
I've wanted to avoid output files being named like Buttonscss.js, like - having some separator seems visually nice
files without extension would have two subsequent . in their name.
Gonna add some special handling for it + tests.
There was a problem hiding this comment.
I've wanted to avoid output files being named like Buttonscss.js, like - having some separator seems visually nice
Definitely, but the point of [extname] vs [ext] should be that [extname] contains a leading . if there is an extension and is an empty string otherwise. So it basically solves exactly this issue. So
| filename | ext | extname | [name][extname].js |
[name].[ext].js |
|---|---|---|---|---|
"test.css" |
"css" |
".css" |
"test.css.js" |
"test.css.js" |
"test" |
"" |
"" |
"test.js" |
"test..js" |
There was a problem hiding this comment.
i'm ok with renaming it though to whatever you see consider fitting here
How about NON_ASSET_EXTENSIONS?
There was a problem hiding this comment.
Definitely, but the point of [extname] vs [ext] should be that [extname] contains a leading . if there is an extension and is an empty string otherwise.
Cool, didn't notice I could use it like this.
How about NON_ASSET_EXTENSIONS?
Sure, sounds good!
| 'output.entryFileNames', | ||
| { | ||
| ext: () => extension.substr(1), | ||
| extname: () => extension, |
There was a problem hiding this comment.
Would be nice to also have a test that directly uses the new pattern elements in entryFileNames. Could also be an extension of an existing test.
|
@lukastaegert thanks for the review, gonna address your concerns in following days and ping you once I'm done with it. |
Andarist
force-pushed
the
preserve-modules/scriptified-assets-ext-in-filenames
branch
from
24fde1a
to
3560bfb
Compare
|
@lukastaegert I believe I've addressed all of your comment |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
fixes #3107
Description
This adds support for
extandextnamereplacements forentryFileNameswhen usingpreserveModules.Using this I include extname in output filenames for "scriptified" assets.