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

fix(index): relax default asset name {RegExp} (options.test) #251

Merged
merged 1 commit into from Mar 7, 2018

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Mar 6, 2018

@alexander-akait alexander-akait added this to the 1.2.3 milestone Mar 6, 2018
@michael-ciniawsky michael-ciniawsky changed the title fix: handle files with query params fix(index): allow query params in asset names Mar 7, 2018
src/index.js Outdated
@@ -20,7 +20,7 @@ class UglifyJsPlugin {

const {
uglifyOptions = {},
test = /\.js$/i,
test = /.((js)?)(\?[a-z0-9]+)?$/i,
Copy link
Member

Choose a reason for hiding this comment

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

The . needs to be escaped (?)

- /.js
+ /\.js...

Why ((js)?) ('nested' capturing groups) ?

@alexander-akait
Copy link
Member Author

@michael-ciniawsky done 👍

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

👍

@michael-ciniawsky michael-ciniawsky changed the title fix(index): allow query params in asset names fix(index): relax default in asset names {RegExp} (options.test) Mar 7, 2018
@michael-ciniawsky michael-ciniawsky merged commit d27e822 into master Mar 7, 2018
@michael-ciniawsky michael-ciniawsky deleted the issue-220 branch March 7, 2018 18:19
@michael-ciniawsky michael-ciniawsky changed the title fix(index): relax default in asset names {RegExp} (options.test) fix(index): relax default asset name {RegExp} (options.test) Mar 7, 2018
@michael-ciniawsky
Copy link
Member

Released in v1.2.3 🎉

@jazdw
Copy link

jazdw commented Mar 10, 2018

@evilebottnawi was it intentional to match the .j extension? Also an empty query should probably be allowed. Maybe \.js(\?.*)?$?

@michael-ciniawsky
Copy link
Member

was it intentional to match the .j extension?

The common default at this stage of the build is a JS asset ending in .js (output.filename/output.chunkFilename), it's possible to override this via options.test in case it's needed (but optional)

Also an empty query should probably be allowed. Maybe .js(?.*)?$?

What's the 'use case' for an empty query? Why/When would someone do this ? But the {RegExp} could eventually be further relaxed/changed if there is a case made :)

@jazdw
Copy link

jazdw commented Mar 11, 2018

I'm not following you, maybe I didn't make myself clear. The Regex in the commit will match files with a .js and a .j extension which I don't think is desirable or the intention.

There's not a use case per say, just in case someone uses a variable substitution that evaluates to an empty string.

@jazdw
Copy link

jazdw commented Mar 13, 2018

@evilebottnawi @michael-ciniawsky any comment on the Regex?

edit. This demonstrates the incorrect behaviour - https://regex101.com/r/SJm7SO/1

@alexander-akait
Copy link
Member Author

@jazdw oh, yes, you are right we should fix it 👍

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

Successfully merging this pull request may close these issues.

None yet

3 participants