Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

refractor filename modify logic #423

Merged
merged 14 commits into from
Feb 26, 2017
Merged

Conversation

lcxfs1991
Copy link
Contributor

  1. Read and sign the CLA. This needs to be done only once. PRs that haven't signed it won't be accepted.
  2. Check out the development guide for the API and development guidelines.
  3. Read through the PR diff carefully as sometimes this can reveal issues. The work will be reviewed, but this can save some effort.
  4. Remove these instructions from your PR as they are for your eyes only.

@lcxfs1991 lcxfs1991 mentioned this pull request Feb 21, 2017
@codecov
Copy link

codecov bot commented Feb 21, 2017

Codecov Report

Merging #423 into master will increase coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #423      +/-   ##
==========================================
+ Coverage   90.25%   90.38%   +0.13%     
==========================================
  Files           6        6              
  Lines         359      364       +5     
  Branches       75       76       +1     
==========================================
+ Hits          324      329       +5     
  Misses         35       35
Impacted Files Coverage Δ
index.js 91.03% <100%> (+0.2%)

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 a078c82...e6c6624. Read the comment docs.

README.md Outdated
@@ -155,6 +155,29 @@ module.exports = {
}
```

### Modify filename

`filename` paramter could be `Object`. It accepts `format` and `modify` callback as attributes.
Copy link
Contributor

Choose a reason for hiding this comment

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

/s/paramter/parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what does that mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a typo.

README.md Outdated
@@ -68,7 +68,7 @@ new ExtractTextPlugin(options: filename | object)
|Name|Type|Description|
|:--:|:--:|:----------|
|**`id`**|`{String}`|Unique ident for this plugin instance. (For advanced usage only, by default automatically generated)|
|**`filename`**|`{String}`|Name of the result file. May contain `[name]`, `[id]` and `[contenthash]`|
|**`filename`**|`{String|Object}`|Name of the result file. May contain `[name]`, `[id]` and `[contenthash]`|
Copy link
Contributor

Choose a reason for hiding this comment

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

It could accept a function directly instead of an object. The function would return a filename and allow you modify it. Simpler API/design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also have to pass filename format like [name].css. Is is possible to do that via a function?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can return a pattern and it will work like before. You can do filename: file => '[name].css' in this design too. And access to the file parameter gives you flexibility for custom logic.

Copy link
Contributor Author

@lcxfs1991 lcxfs1991 Feb 21, 2017

Choose a reason for hiding this comment

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

If you add js/a as the entry key, webpack will replace [name] with css/js/a and it leaves no room for developers to do the following action:

filename.replace('js/css', 'css');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can try my test case:

test/cases/multiple-entries-filename/webpack.config.js

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. It's that compilation.getPath which is the problem. Basically modify triggers after that.

The design could be inverted like this:

filename: (getPath, filename) => getPath(filename)

That getPath would run compilation.getPath with all its parameters and provide the hook that would give you maximum amount of control. Now you could manipulate filename before and after placeholders get applied while keeping the API surface minimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expose getPath is cool too.

let me retune it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

look good?

extra

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'll have a closer look in a bit. 👍

@bebraw bebraw added this to the 2.1 features/fixes milestone Feb 21, 2017
@bebraw
Copy link
Contributor

bebraw commented Feb 21, 2017

This looks good to me.

Please resolve that conflict (had to merge a readme fix). I'll merge this for 2.1 (first release after final 2.0).

@bebraw
Copy link
Contributor

bebraw commented Feb 26, 2017

@d3viant0ne Do you want to take a quick peek? Looks good to me for a quick 2.1.

Copy link
Member

@joshwiens joshwiens left a comment

Choose a reason for hiding this comment

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

Nice work @lcxfs1991 - Ship it :)

@bebraw bebraw merged commit c9a19ad into webpack-contrib:master Feb 26, 2017
@bebraw
Copy link
Contributor

bebraw commented Mar 5, 2017

Published as 2.1.0. Sorry for the delay. Busy weeks.

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

Successfully merging this pull request may close these issues.

3 participants