feat: allow filename to be a `{Function}` #145

berthertogen · 2018-05-14T11:49:08Z

This PR contains a:

Motivation / Use-Case

closes #143

Breaking Changes

Additional Info

Wasn't able to get the tests to run! But wrote a test for the filename-function.

jsf-clabot · 2018-05-14T11:49:09Z

All committers have signed the CLA.

alexander-akait · 2018-05-14T11:50:15Z

src/index.js

@@ -111,6 +111,11 @@ class MiniCssExtractPlugin {
      options
    );
    if (!this.options.chunkFilename) {
+      if (this.isFunction(this.options.filename)) {
+        throw new Error(
+          `You are required to provide a value for chunkFilename when using a Function for this.options.filename, please specify the [name], [id] or [chunkhash] in this function`


Remove this throw, it should be done after we integrate schema for options

alexander-akait · 2018-05-14T11:52:28Z

src/index.js

+    return this.isFunction(filename) ? filename(chunk) : filename;
+  }
+
+  isFunction(functionToCheck) {


Remove this and use typeof something === 'function'

Jessidhia

This looks OK, but I'm not sure it's a good idea to call getFilename multiple times. It's possible the callback has side-effects (say, incrementing a counter) and returns different results every time.

There's also the leftover console.logs ;)

aaarichter · 2018-05-22T17:13:45Z

looking forward to this

alexander-akait · 2018-05-22T18:09:45Z

src/index.js

@@ -366,6 +371,16 @@ class MiniCssExtractPlugin {
    });
  }

+  getFilename(chunk, filename) {
+    console.log('--chunk--');
+    console.log(chunk);


Remove console.log

Store filename in options, this way function is called once

berthertogen · 2018-05-25T18:16:57Z

Sorry it took so long, I remove the console logs and made sure the filename function is called only once.

glen-84 · 2018-06-16T07:43:04Z

Is there enough information in chunk to figure out the path used to reference the CSS file/the path on disk? (see #92)

ooflorent · 2018-06-16T12:11:28Z

src/index.js

+              filenameTemplate: this.getFilename(
+                chunk,
+                this.options.filename,
+                this.options.processedFilename


What is this option? There is no documentation about it and it is not used by the test cases.

ooflorent · 2018-06-16T12:12:00Z

src/index.js

@@ -366,6 +380,19 @@ class MiniCssExtractPlugin {
    });
  }

+  getFilename(chunk, filename, processedFilename) {
+    if (!processedFilename) {
+      processedFilename = this.isFunction(filename)


Use return here. Reassigning parameters result in deopt.

Could you use typeof instead please?

I second the parameter reassign, and did not include it in my review because it is mentioned here. this should not be done, and the linter should be throwing a warning there about it. Run npm lint before the next commit to assure all warnings have been mitigated.

shellscape

My comments and review are more geared towards code quality, which I feel can be improved to reduce long-term technical debt.

shellscape · 2018-06-16T12:10:55Z

src/index.js

@@ -260,7 +268,13 @@ class MiniCssExtractPlugin {
          if (Object.keys(chunkMap).length > 0) {
            const chunkMaps = chunk.getChunkMaps();
            const linkHrefPath = mainTemplate.getAssetPath(
-              JSON.stringify(this.options.chunkFilename),
+              JSON.stringify(


I'd like to see a "safe" stringify done here, or at the very least this done in a try/catch that can safely trap and display a meaningful error message.

shellscape · 2018-06-16T12:11:36Z

src/index.js

@@ -260,7 +268,13 @@ class MiniCssExtractPlugin {
          if (Object.keys(chunkMap).length > 0) {
            const chunkMaps = chunk.getChunkMaps();
            const linkHrefPath = mainTemplate.getAssetPath(


we've got three nested calls here. that's a nightmare for debugging. break these values out into separate consts and pass the variables to each function.

shellscape · 2018-06-16T12:12:10Z

src/index.js

+              JSON.stringify(
+                this.getFilename(
+                  chunk,
+                  this.options.chunkFilename,


use destructuring instead of repeating this.options. the same comment applies to several places in this file.

shellscape · 2018-06-16T12:16:51Z

src/index.js

@@ -366,6 +380,19 @@ class MiniCssExtractPlugin {
    });
  }

+  getFilename(chunk, filename, processedFilename) {


Is filename actually a full or partial path? If so, follow the node convention and call this path.

If processedFilename the path after a transformation? If so, then this should be the path/filename and the non-transformed path/filename should be the resultPath/resultFilename.

I'd like to see this use a single opts parameter, and the signature change to { chunk, fileName, resultFileName }, or if filename is actually a path, { chunk, path, resultPath }

shellscape · 2018-06-16T12:19:06Z

src/index.js

@@ -366,6 +380,19 @@ class MiniCssExtractPlugin {
    });
  }

+  getFilename(chunk, filename, processedFilename) {
+    if (!processedFilename) {


we don't need to do an if with a ternary here:

processedFilename = processedFilename || this.isFunction(filename) ? filename(chunk) : filename;

shellscape · 2018-06-16T12:20:04Z

src/index.js

+    return processedFilename;
+  }
+
+  isFunction(functionToCheck) {


we don't need a separate function here, because it's only called once. Replace the isFunction call with the typeof check instead.

michael-ciniawsky · 2018-08-21T20:31:39Z

Closing in favor of #225

Thx :)

feat: allow filename to be a function

85f6eda

alexander-akait requested changes May 14, 2018

View reviewed changes

berthertogen added 4 commits May 14, 2018 14:05

feat: update tests

52cf7f6

feat: remove throw and change isFunction

cf7b731

feat: pass correct chunk

a06f5df

feat: fix test and names

835624c

alexander-akait approved these changes May 17, 2018

View reviewed changes

Jessidhia reviewed May 17, 2018

View reviewed changes

alexander-akait requested changes May 22, 2018

View reviewed changes

feat: remove console.log's

44cd88f

Store filename in options, this way function is called once

glen-84 mentioned this pull request Jun 16, 2018

Can filename/chunkFilename support function type ? #67

Closed

alexander-akait approved these changes Jun 16, 2018

View reviewed changes

alexander-akait requested review from shellscape and ooflorent June 16, 2018 12:05

ooflorent reviewed Jun 16, 2018

View reviewed changes

shellscape suggested changes Jun 16, 2018

View reviewed changes

michael-ciniawsky changed the title ~~feat: allow filename to be a function~~ feat: allow filename to be a {Function} Aug 6, 2018

michael-ciniawsky added this to the 0.5.0 milestone Aug 6, 2018

michael-ciniawsky added type: feature semver: minor pr: review status: pending labels Aug 6, 2018

michael-ciniawsky mentioned this pull request Aug 7, 2018

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

Closed

michael-ciniawsky closed this Aug 21, 2018

michael-ciniawsky added the status: declined label Aug 21, 2018

michael-ciniawsky removed pr: review status: pending labels Aug 21, 2018

michael-ciniawsky removed this from the 0.5.0 milestone Aug 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow filename to be a `{Function}` #145

feat: allow filename to be a `{Function}` #145

berthertogen commented May 14, 2018 •

edited

Loading

jsf-clabot commented May 14, 2018 •

edited

Loading

alexander-akait May 14, 2018

alexander-akait May 14, 2018

Jessidhia left a comment •

edited

Loading

aaarichter commented May 22, 2018

alexander-akait May 22, 2018

berthertogen commented May 25, 2018

glen-84 commented Jun 16, 2018

ooflorent Jun 16, 2018

ooflorent Jun 16, 2018

ooflorent Jun 16, 2018

shellscape Jun 16, 2018

shellscape left a comment

shellscape Jun 16, 2018

shellscape Jun 16, 2018

shellscape Jun 16, 2018

shellscape Jun 16, 2018

shellscape Jun 16, 2018

shellscape Jun 16, 2018

michael-ciniawsky commented Aug 21, 2018

feat: allow filename to be a {Function} #145

feat: allow filename to be a {Function} #145

Conversation

berthertogen commented May 14, 2018 • edited Loading

Motivation / Use-Case

Breaking Changes

Additional Info

jsf-clabot commented May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jessidhia left a comment • edited Loading

Choose a reason for hiding this comment

aaarichter commented May 22, 2018

Choose a reason for hiding this comment

berthertogen commented May 25, 2018

glen-84 commented Jun 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shellscape left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky commented Aug 21, 2018

feat: allow filename to be a `{Function}` #145

feat: allow filename to be a `{Function}` #145

berthertogen commented May 14, 2018 •

edited

Loading

jsf-clabot commented May 14, 2018 •

edited

Loading

Jessidhia left a comment •

edited

Loading