-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Correctly skip plus-prefixed URLs #21
Conversation
👍 |
@sindresorhus Either way, would you prefer a more compatible solution? I'm thinking something like explicitly matching the prepended |
Hello 😄 |
I'm ok with targeting Node.js 10 to be able to use lookbehinds. |
@ruipneves You're right, only Chrome seems to support this. Sidenote: Here's what I did, IIRC:
So, how exactly should my workflow have been for this? |
The lookbehind was more elegant but the current PR is indeed more widely supported in terms of browsers. @sindresorhus can you give your opinion regarding the current state of the PR vs the lookbehind strategy? Bear in mind that there's no babel plugin able to transform lookbehind for older browsers, see: http://kangax.github.io/compat-table/es2016plus/#test-RegExp_Lookbehind_Assertions |
@pmpinto regarding the conflicts, try the following:
|
I still prefer the lookbehind approach. This is a niche module and I would prefer to keep it as simple as possible. I'm not that concerned with browser support. We can add a note to the readme to stay on v2 if people needs to support different browsers. |
How: Adding a negative lookbehind on the main RegEx to ignore URLs preceded by Why: Negative lookbehinds seem to be supported since Node v6 with the flag (ref: https://node.green/#ES2018-features--RegExp-Lookbehind-Assertions) and should also be supported in ES2019 (ref: https://tc39.github.io/ecma262/).
f9156de
to
2ee8caa
Compare
@satazor Thanks for the Git tips! I'm still not confident by looking at the whole commit history, but the current state is the intended one. @sindresorhus Thanks for your input, just updated the RegEx, tests and README file. |
Even though tests were fine
@pmpinto It would have been a good practice to rebase all the commits to just one by using |
@pmpinto considering that @sindresorhus is ok with not supporting |
@vasco-santos Thanks for the heads up, Vasco! |
I actually prefer to keep the history intact to more easily see what changed. I squash on merge regardless. |
Looks good! Nice work, @pmpinto :) |
Should fix #7
How: Adding a negative lookbehind on the main RegEx to ignore URLs preceded by
Why: Negative lookbehinds seem to be supported since Node v6 with the
--harmony
flag (ref: https://node.green/#ES2018-features--RegExp-Lookbehind-Assertions) and should also be supported in ES2019 (ref: https://tc39.github.io/ecma262/).Please note that it doesn't specifically ignore URLs that start with
git+
, just the+
is being taken into account.So I would suggest 1 of 2 changes:
If it makes sense to apply this same rule for any other URLs that might follow the same schema but not exactly git-specific I would suggest (a) tweak the
skips Git URLs
test name itself. Otherwise, (b) the RegEx should be tweaked to explicitly ignoregit+
URLs.Let me know what you think.