Skip to content
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

add exclusions for absolute URLs #35

Merged

Conversation

thescientist13
Copy link
Contributor

@thescientist13 thescientist13 commented Sep 13, 2019

resolves #21 by adding exceptions for paths that wouldn't need cdnizing, like:

  • //cdn.com/lib/index.js
  • http[s]://cdn.com/lib/index.js

@OverZealous
Copy link
Owner

Thanks for taking a look at this. As you can probably guess, I haven't really been maintaining this repo for a while now.

I think this is an interesting idea, but I have a couple requests:

  1. It should be optional, and not enabled by default. It might be weird, but it's already something that can be fixed with tighter Regexes, and there's always the possibility someone wants to swap absolute URLs with a different one. It also means I don't have to release this as a major version.
  2. The protocol check should include the colon (':'), I'll make a note inline in the code about this.
  3. Could you squash your commits down if you make the above changes?

@thescientist13
Copy link
Contributor Author

thescientist13 commented Sep 17, 2019

Thanks for taking a look and happy to make the suggested changes.

but it's already something that can be fixed with tighter Regexes

fair point, but happy that this can be considered a feature of the tool 😄

regarding this part

It should be optional, and not enabled by default

To implement that, would you like it so the user just passes something like

processInput({
  files: ['**/*.js', '**/*.png', '**/*.css'],
  defaultCDNBase: '//examplecdn/',
  preserveAbsolute: true
}, 'index-exclusions.html', 'index-exclusions.html');

and then the tool would only enable that check if the flag is in place?

var cdnTemplate = preserveAbsolute && excludeCdnPrefix 
  ? '<%= filepath %>' 
  : fileInfo.cdn || opts.defaultCDN;

I can update the README as well if the above behavior documented.

@OverZealous
Copy link
Owner

Yeah, exactly! And if you would update the README, that would be great, too. Thanks a bunch!

@thescientist13
Copy link
Contributor Author

@OverZealous
Done! I'll squash once everything looks good to you. 👍

Copy link
Owner

@OverZealous OverZealous left a comment

Choose a reason for hiding this comment

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

Looks great!

clarify test case

refactor

fix formatting

fix formatting

fix formatting

fix formatting

enable feature as an opt-in flag and document

grammar

grammar

grammar
@thescientist13 thescientist13 force-pushed the enhancement/issue-21-exclusions branch from d816251 to e40afa4 Compare September 17, 2019 19:16
@thescientist13
Copy link
Contributor Author

@OverZealous
Squashed 🔨

@OverZealous OverZealous merged commit 84a1282 into OverZealous:master Sep 17, 2019
@thescientist13 thescientist13 deleted the enhancement/issue-21-exclusions branch September 17, 2019 19:20
@OverZealous
Copy link
Owner

Published as v3.1.0

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

Successfully merging this pull request may close these issues.

Excluding absolute url
2 participants