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

feat(index): allow filename to be a {Function} (options.filename) #225

Closed

Conversation

owlyowl
Copy link

@owlyowl owlyowl commented Aug 3, 2018

Clean up as per feedback by @shellscape

Issues

@jsf-clabot
Copy link

jsf-clabot commented Aug 3, 2018

CLA assistant check
All committers have signed the CLA.

@owlyowl owlyowl force-pushed the feat/143-filename-function branch 14 times, most recently from 945943f to 378c955 Compare August 3, 2018 08:18
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Please describe use case, also please improve docs

@owlyowl
Copy link
Author

owlyowl commented Aug 6, 2018

@evilebottnawi

This PR contains a:
new feature
test update

Motivation / Use-Case
Updated Readme.md to include a use case.

closes #143 and implements improvements as per @shellscape feedback here: https://github.com/webpack-contrib/mini-css-extract-plugin/pull/145

With the deprecation of ExtractTextPlugin for Webpack 4 we need to implement this feature to be at parity with what we had in ExtractTextPlugin with regards to filename options.

This is handy for multiple entry point implementation.

@michael-ciniawsky michael-ciniawsky changed the title Feat/143 filename function - Clean up as per feedback by @shellscape feat: allow filename to be a {Function} Aug 6, 2018
@michael-ciniawsky michael-ciniawsky added this to the 0.5.0 milestone Aug 6, 2018
@danechitoaie
Copy link

Any idea when this will be released. Really needing this feature.

@dankanze
Copy link

Status?

@alexander-akait
Copy link
Member

Need rebase

@gianlucalarizza
Copy link

Hi, I've been waiting for this feature . it would be nice if it were released. I am using Extract Text Plugin in beta for webpack 4 and I would be happy to dismiss it as soon as possible. Many many thanks

shortChunkHashMap[chunkId] = chunkMaps.hash[
let chunkFilename;
try {
chunkFilename = JSON.stringify(
Copy link
Member

Choose a reason for hiding this comment

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

That's incorrect. chunkFilename can't be a function. The chunkFilename generation logic need to be embedded into the runtime code.

chunk is the runtime chunk here and the referenced chunk is not known.

plugins: [
new Self({
filename: (chunk) => chunk.name == 'app' ? 'this.is.app.css' : `[name].css`,
chunkFilename: (chunk) => '[name].css',
Copy link
Member

Choose a reason for hiding this comment

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

chunkFilename can't be a function

@@ -170,7 +170,7 @@ class MiniCssExtractPlugin {
renderedModules,
compilation.runtimeTemplate.requestShortener
),
filenameTemplate: this.options.filename,
filenameTemplate: this.getFilename(chunk, this.options.filename),
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can pass a function to filenameTemplate too. webpack will call it with an information object.

@@ -194,7 +194,10 @@ class MiniCssExtractPlugin {
renderedModules,
compilation.runtimeTemplate.requestShortener
),
filenameTemplate: this.options.chunkFilename,
filenameTemplate: this.getFilename(
Copy link
Member

Choose a reason for hiding this comment

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

Same

@@ -366,6 +378,10 @@ class MiniCssExtractPlugin {
});
}

getFilename(chunk, filename) {
Copy link
Member

Choose a reason for hiding this comment

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

Not needed

@gianlucalarizza
Copy link

Dear @sokra,
at what point is the level of approval of the request? Do you think it can be released in times. For many of us, using the filename option as a function is really very important to choose this plugin for Webpack 4. Thank you for your work

@pawelwieladek
Copy link

Dear @owlyowl
My team is waiting for this feature. I would be grateful if you release it. Thanks!

@gianlucalarizza
Copy link

gianlucalarizza commented Dec 6, 2018

Dear @owlyowl
My team is waiting for this feature. I would be grateful if you release it. Thanks!

my team is also waiting for this release.
We are using extract-text-webpack-plugin 4.0 beta version, but since webpack v4 the extract-text-webpack-plugin should not be used for css. :(

@alexander-akait
Copy link
Member

Feel free to create new PR looks this abadoned

@kevinchappell
Copy link
Contributor

@evilebottnawi I just saw this PR after opening #321. I'll use the comments here as a guide to improve #321.

@domonique-ncino
Copy link

Can't wait until this is merged!!!

@alexander-akait
Copy link
Member

Need rebase

@jrjacobs24
Copy link

Is there any update on when this is expected to get merged? Until then, I found a workaround that seemed to work well for me. I originally had a function for the filename option working, then for some reason after a bit of cleanup I started getting an error and my build would fail. The original error I was seeing was:

TypeError: filename.includes is not a function
    at new MiniCssExtractPlugin (.../node_modules/mini-css-extract-plugin/dist/index.js:139:32)

So I jumped into the code to get more context as to why it was failing and found the line that was causing the error was wrapped in a conditional check for the chunkFileName on ln129. That's when it hit me that the change that I had made that caused it to fail was me removing the chunkFileName because I didn't think I needed it.

Apparently I do though until this PR gets merged. But for those in the same boat I was in, utilizing the chunkFileName option may do the trick for you:

new MiniCssExtractPlugin({
  filename: (chunkData) => {
    const name = chunkData.chunk.name.replace('js/build/', '').replace('components', 'base');

    return `scss/build/${name}-theme.css`;
  },
  chunkFilename: 'scss/build/[name]-theme.css',
})

@alexander-akait
Copy link
Member

@jrjacobs24 PR was abandoned, can you send it again?

@jrjacobs24
Copy link

@evilebottnawi What did you need me to send again? I didn't have anything to do with this PR originally, but I'm happy to help see it through if needed 👍

@alexander-akait
Copy link
Member

👍 Need resend this PR

@shanecav84
Copy link

@evilebottnawi Does #381 do the same thing as what's being requested?

@alexander-akait
Copy link
Member

@shanecav84 no

@shanecav84
Copy link

These look to have overlapping functionality:

moduleFilename (in master):

this.options = Object.assign(
  {
    filename: DEFAULT_FILENAME,
    moduleFilename: () => options.filename || DEFAULT_FILENAME,
  },
  options
)
...
filenameTemplate: ({ chunk: chunkData }) =>
  this.options.moduleFilename(chunkData)

filename (this PR):

this.options = Object.assign({ filename: '[name].css' }, options)
...
filenameTemplate: this.getFilename(chunk, this.options.filename)
...
getFilename(chunk, filename) {
  return typeof filename === 'function' ? filename(chunk) : filename;
}

@alexander-akait
Copy link
Member

@shanecav84 one option for non async chunk other option for async chunk, sometimes developers need this, but i agree we should simplify this, PR welcome

@shanecav84
Copy link

I could help. I'm still not understanding though.

one option for non async chunk other option for async chunk

Which option is which?

Why not just have filename accept a string or a function (like webpack), and point moduleFilename to filename for backward compatibility?

this.options = Object.assign(
  {
    filename: options.filename || DEFAULT_FILENAME,
    moduleFilename: () => options.filename,
  },
  options
)
...
filenameTemplate: typeof this.options.filename === 'function' 
  ? this.options.filename(chunk) 
  : filename

@alexander-akait
Copy link
Member

@shanecav84 filename !== moduleFilename in some cases, but we can use filename for moduleFilename by default, PR welcome

@shanecav84
Copy link

shanecav84 commented Jul 1, 2019

I'm working on rebasing this branch on master. The conflict is here, where both filename and moduleName are being used to set filenameTemplate. Which should we use?

@owlyowl
Copy link
Author

owlyowl commented Jul 2, 2019

Sorry for the delay in sorting this PR out. Thanks @shanecav84 and @evilebottnawi
If you let me know how you'd like the conflict resolved I'm happy to sort it out..

I've tried using the @jrjacobs24 workaround without success as I'm generating some theme based style sheets in production. What it results in is making a bunch of css files with hashes in the names that are webpackbootstrap JS so any subsequent css processing fails as the css files actually contain javascript.. see here:

See Here

I'd love to know how to fix this it is driving me insane. Having this feature PR'd in would sort it out I suspect but master has moved out from under me sorry. @evilebottnawi any idea how you'd like the merge conflict corrected?

@codecov
Copy link

codecov bot commented Apr 23, 2020

Codecov Report

Merging #225 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #225   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           3       3           
  Lines         241     246    +5     
  Branches       49      50    +1     
======================================
- Misses        198     202    +4     
- Partials       43      44    +1     
Impacted Files Coverage Δ
src/index.js 0.00% <0.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 41347f7...6012a49. Read the comment docs.

@alexander-akait
Copy link
Member

Supported

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

Successfully merging this pull request may close these issues.

Can filename/chunkFilename support function type ?