-
Notifications
You must be signed in to change notification settings - Fork 93
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
Fixed #241 add new option newLineExemptFileExts #248
Fixed #241 add new option newLineExemptFileExts #248
Conversation
… extensions array/csv
I have a proposed fix for #241 here. Let me know if this would meet the needs. Css files don't really need the newline character at all. But it's been there a long time and I don't want to break anybody's code. |
I discovered issue #241 myself in a legacy project I was working on, when after several hours I had a Homer moment when I realised semicolons I had set to fix the JS bundle were being inserted into the CSS bundle too. Originally, I coded a simple fix, but then realised this plugin is used with a wide range of stacks, not just the usual HTML/CSS/JS, which is why this PR, is an additive non-breaking change, that allows more flexibility to accommodate other use cases/stacks I'm not aware of. The ultimate flexible solution, although I'm not sure is required, would be a hash table option to allow 'newLine' character(s)/codes to be set per file extension. |
That's the thing. I can't find any instances of anyone using a newline character to combine css files. I see it more as fixing a bug to allow the feature to be used correctly. I would rather take that approach before coding a feature that is not needed. |
I've just had a quick look again at PR #249. This condition uses an OR operator. I assume this should be an AND operator?
I agree a fix should be kept simple, but PR #249, (assumption above), will only apply The problem is, I cannot be sure how gulp-useref is used in the wild and for which file types. This is why PR #248 is flexible enough to allow users to continue without making any changes but gives them the opportunity to apply or not apply the |
Ok, I agree with this. I've updated #249. |
#249 is merged. |
I've added 1 new option:
newLineExemptFileExts
(optional), which may be passed in addition to thenewLine
option which specifies which file extensions should ignore thenewLine
option. It uses a case insensitive RegExp to match the file extensions, with/without the leading full stop/period.I've added 2 new passing tests, but had some trouble with the test suite, as when a test failed, all the remaining tests would fail, but only after timing out. I'm not sure if this is the expected behaviour, or if there is anything I should/can do?