feat: add option to merge files (`options.merge`) #636

danikaze · 2017-09-29T02:18:17Z

Implements #634

Current status: it's merging files properly but the map file is lost in the intermediate step.

source1 --> file1.css + map1
source2 --> file2.css + map2 --> mergedFile (no map)
source3 --> file3.css + map3

jsf-clabot · 2017-09-29T02:18:18Z

All committers have signed the CLA.

codecov · 2017-09-29T02:28:09Z

Codecov Report

Merging #636 into master will increase coverage by 0.53%.
The diff coverage is 93.93%.

@@            Coverage Diff             @@
##           master     #636      +/-   ##
==========================================
+ Coverage   87.41%   87.95%   +0.53%     
==========================================
  Files           7        7              
  Lines         302      332      +30     
  Branches       68       73       +5     
==========================================
+ Hits          264      292      +28     
- Misses         36       38       +2     
  Partials        2        2

Impacted Files	Coverage Δ
src/index.js	`90.36% <93.93%> (+0.65%)`	⬆️

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 8de6558...cccabaa. Read the comment docs.

michael-ciniawsky

Thx the logic looks good so far, but needs tests an interface design triage

michael-ciniawsky · 2017-09-29T02:49:00Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "extract-text-webpack-plugin",
-  "version": "3.0.0",
+  "version": "3.1.0",


Please revert this as we have release management in place and will take care of that

michael-ciniawsky · 2017-09-29T02:53:01Z

src/index.js

+                  this.filesToMerge[mergeChunk.filename] = [];
+                }
+                this.filesToMerge[mergeChunk.filename].push(extractedChunk);
+                preventOutput = preventOutput || (mergeChunk.preventOriginalOutput !== false);


Can we find a better name (less verbose) then preventOriginalOutput => prevent || original || preventOriginal || ? ?

sure, I was thinking also about that :P

I will include the tests, but wanted to make sure it wasn't going to become a dropped feature :)

😛 Well we definitely need to check for ordering issues && multi entry setups and it needs approval from other maintainers aswell. But the feature itself is a nice improvement imho

…rge[].originals

…ons.merge[].originals * Revert version number to 3.0.0 due to internal management * Renamed options.merge[].preventOriginalOutput to a shorter options.merge[].originals * Test added but breaking lol. Need to check generated file names * Added test, but can't debug it * Need to check merge.originals option * options.merge[].originals working now

… should output original files

danikaze · 2017-10-02T01:59:40Z

Hello. It looks like test is passing now.
But it took me some time so I want to comment about the environment:

jest is buggy. It doesn't allow any console.log to be printed. It's a known bug. Any workaround?
Tests were running and passing in local environment, but not in appveyor.
It's hard to debug because the running setup doesn't allow for breakpoints (or not easily) + no console.log = impossible to follow while running.

So, after just checking the logic, I made it work.
But travis is failing because of this

ERROR: npm is known not to run on Node.js v4.3.2
Node.js 4 is supported but the specific version you're running has
a bug known to break npm. Please update to at least ${rel.min} to use this
version of npm. You can find the latest release of Node.js at https://nodejs.org/

Anything to do?

michael-ciniawsky · 2017-10-02T03:18:07Z

jest is buggy. It doesn't allow any console.log to be printed. It's a known bug. Any workaround?

Could you post the e.g tracking issue or elaborate where/what isn't going to work? I use console.logwith Jest on a regular basis and never have had any issue, but the ETWP code base might be different here :)

Does npm t -- --verbose work at least?

Tests were running and passing in local environment, but not in appveyor

I will take a look later

But travis is failing because of this

Ignore this for now as it is unrelated to ETWP or this PR in particular (it's a known npm issue with node =< v4.5.0)

danikaze · 2017-10-02T04:08:27Z

jest bug: jestjs/jest#2166
It's not important, but when things doesn't work, it's hard to debug.

appveyor is passing now (after the last commit) so it should be ready to merge if we ignore the npm 4.3 issue in travis.
The main problem is, I was having a false positive in my local environment, don't know why because breakpoints weren't working.

joshwiens

I want @sokra's take on this before it goes anywhere. Given this ( etwp ) is going away & being rolled back into Webpack, any feature added is one that has to be ported into Webpack.

My initial impression is conceptually, this is a bit murky, hard to sufficiently test & I guarantee people are going to have trouble with it. That said, if Tobias is okay with it conceptually then we can work through the implementation .

joshwiens · 2017-10-03T14:49:43Z

@danikaze - We are going to have to pass on this one. Even if there wasn't already a plugin published for this, the last thing etwp needs at this point is more complexity.

This is already planned for retirement & the functionality rolled back into Webpack where it belongs, this particular use case may or may not be addressed at that time, it's not for me to say.

danikaze · 2017-10-03T15:03:11Z

I see. Bad luck then.
I thought after finding lot of people looking for this feature and an outdated plugin, that would be nice to have this in the core of the plugin, but I see what you mean.
I´ll try to move the logic then to yet another plugin, though it would be harder to generate map files in a complete different step, I think.

joshwiens · 2017-10-03T15:21:03Z

@danikaze - If you feel there is enough demand for this particular plugin, we can discuss rolling it as a stand alone in contrib.

Given the state of etwp, where it's going & the similarities with the allChunks setting this isn't the right place for it.

danikaze · 2017-10-04T14:24:46Z

@d3viant0ne sure.
Thanks for your time taking a look anyways ;)

michael-ciniawsky changed the title ~~implementation of options.merge (#634)~~ feat: add option to merge files (options.merge) Sep 29, 2017

michael-ciniawsky added pr_action: Review semver: Minor status: Pending type: Feature labels Sep 29, 2017

michael-ciniawsky added this to the 3.1.0 milestone Sep 29, 2017

michael-ciniawsky requested review from alexander-akait, michael-ciniawsky and joshwiens September 29, 2017 02:47

michael-ciniawsky reviewed Sep 29, 2017

View reviewed changes

michael-ciniawsky added the flag: Needs Tests label Sep 29, 2017

danikaze force-pushed the master branch from 57fb4b8 to e9dc8cf Compare September 29, 2017 06:16

Implementation of options.merge (webpack-contrib#634)

2036391

danikaze force-pushed the master branch from e9dc8cf to 2036391 Compare September 29, 2017 06:19

danikaze added 6 commits September 29, 2017 15:27

Revert version number to 3.0.0 due to internal management

9aec112

Renamed options.merge[].preventOriginalOutput to a shorter options.me…

cf03ee8

…rge[].originals

Merge branch 'master' of github.com:danikaze/extract-text-webpack-plugin

2b63b7d

Added test/__snapshots__/ after the test is passing for options.merge

b449cb6

If there's no options.merge or don't match any options.merge.test, it…

cccabaa

… should output original files

joshwiens added pr_action: Discuss and removed pr_action: Review labels Oct 3, 2017

joshwiens reviewed Oct 3, 2017

View reviewed changes

joshwiens added status: Declined and removed pr_action: Discuss labels Oct 3, 2017

joshwiens removed semver: Minor status: Pending labels Oct 3, 2017

joshwiens closed this Oct 3, 2017

joshwiens mentioned this pull request Oct 3, 2017

Generate one (or more) resulting files merging the original results #634

Closed

michael-ciniawsky added semver: Minor and removed flag: Needs Tests labels Oct 3, 2017

michael-ciniawsky removed this from the 3.1.0 milestone Oct 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add option to merge files (`options.merge`) #636

feat: add option to merge files (`options.merge`) #636

danikaze commented Sep 29, 2017

jsf-clabot commented Sep 29, 2017 •

edited

Loading

codecov bot commented Sep 29, 2017 •

edited

Loading

michael-ciniawsky left a comment

michael-ciniawsky Sep 29, 2017

danikaze Sep 29, 2017

michael-ciniawsky Sep 29, 2017

danikaze Sep 29, 2017

michael-ciniawsky Sep 29, 2017

danikaze commented Oct 2, 2017 •

edited

Loading

michael-ciniawsky commented Oct 2, 2017

danikaze commented Oct 2, 2017

joshwiens left a comment

joshwiens commented Oct 3, 2017

danikaze commented Oct 3, 2017

joshwiens commented Oct 3, 2017

danikaze commented Oct 4, 2017

feat: add option to merge files (options.merge) #636

feat: add option to merge files (options.merge) #636

Conversation

danikaze commented Sep 29, 2017

jsf-clabot commented Sep 29, 2017 • edited Loading

codecov bot commented Sep 29, 2017 • edited Loading

Codecov Report

michael-ciniawsky left a comment

Choose a reason for hiding this comment

michael-ciniawsky Sep 29, 2017

Choose a reason for hiding this comment

danikaze Sep 29, 2017

Choose a reason for hiding this comment

michael-ciniawsky Sep 29, 2017

Choose a reason for hiding this comment

danikaze Sep 29, 2017

Choose a reason for hiding this comment

michael-ciniawsky Sep 29, 2017

Choose a reason for hiding this comment

danikaze commented Oct 2, 2017 • edited Loading

michael-ciniawsky commented Oct 2, 2017

danikaze commented Oct 2, 2017

joshwiens left a comment

Choose a reason for hiding this comment

joshwiens commented Oct 3, 2017

danikaze commented Oct 3, 2017

joshwiens commented Oct 3, 2017

danikaze commented Oct 4, 2017

feat: add option to merge files (`options.merge`) #636

feat: add option to merge files (`options.merge`) #636

jsf-clabot commented Sep 29, 2017 •

edited

Loading

codecov bot commented Sep 29, 2017 •

edited

Loading

danikaze commented Oct 2, 2017 •

edited

Loading