-
Notifications
You must be signed in to change notification settings - Fork 65
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
Update path regex to not accept closing parens #35
base: master
Are you sure you want to change the base?
Conversation
This fixes a few cases: * css url's of the format "background-image:url(http://example.com/img.jpg);" * markdown url's such as "look at [my image](http://example.com/img.jpg) blah blah"
Parentheses at the end of URLs are valid though and commonly used by for example Wikipedia. We need to check if the URL starts with a parens too and then skip the closing one. Until we have lookbehind assertions, my best advice is just to remove them manually. |
@kevva Maybe you can simulate lookbehind: https://stackoverflow.com/questions/7376238/javascript-regex-look-behind-alternative Also see the JavaScript section in https://stackoverflow.com/a/35271017/64949 Could maybe use |
It's almost impossible to take all edge cases into account while still following the spec. It's also somewhat common in PHP to use the following syntax when passing arrays into query strings.
The spec says the following though:
|
@kevva Since we can't easily do anything about it right now, I think we should at least give the user a choice, so we should add an option whether to match parens or not. For my use-case, I'm ok with not supporting a few Wikipedia links with parens in favor of not matching Markdown syntax. |
Happy to. Do you think it fits better as a standalone option or using it with the |
Standalone option. |
Lookbehinds are now in Chrome beta and will be out in Chrome stable in Oktober and probably come to Node.js this year. We could feature detect lookbehinds and use it when possible. We need parens support for Refined GitHub and we can target latest Chrome. // @bfred-it |
Cool! I've keeping an eye on any |
@kevva Yeah, I'm not surprised. It's very difficult to polyfill lookbehind. |
Chrome 62 is out with lookbehind support. I suggest implementing lookbehind support behind an option for now. |
Lookbehind support is available in Node.js 10, so I suggest resolving this with lookbehinds, target Node.js 10, and do a major version bump. |
For anyone stumbling on that, you can clean up the results from const someLink = "[][][][][[[]])())))]]]google.com][[](()()";
someLink.replace(/^[\[\]\(\)]*|[\[\]\(\)]*$/g, ''); // google.com |
…s on normalized links parsed from url-regex per <kevva/url-regex#35>
Another issue for folks to be aware of is that URL's returned from this regex will include stuff like this:
|
The fix for my previous comment is this: let someLink = "{background-image:url('someurl.com/some/path";
const quoteIndex = someLink.lastIndexOf('"');
const apostropheIndex = someLink.lastIndexOf("'");
const index = apostropheIndex > quoteIndex ? apostropheIndex : quoteIndex;
if (index !== -1) someLink = someLink.substring(index + 1); // 'someurl.com/some/path' |
This issue is fixed in my maintained and modern version of this package at https://github.com/niftylettuce/url-regex-safe. You should be able to switch from |
This fixes a few cases, one of which is documented in #34:
background-image:url(http://example.com/img.jpg);
look at [my image](http://example.com/img.jpg) blah blah