-
-
Notifications
You must be signed in to change notification settings - Fork 314
root_domain fails on "unusual" URI-schemes #98
Comments
First, this is somewhat related to #93. Also, your proposed regex doesn't actually work: /^[a-z][\w\+-\/]+\:\/\/\/?[^\/]+/.test("chrome-extension:mmjeglkkcnglojkjoomifokhekiiiioe/www/index.html"); // false We'd have to tweak the regex to make the Moreover, the question is: could we actually assume to detect against URLs that don't look like normal web URL's and not break other important behavior (namely absolute/relative URL handling)? In addition to the But, imagine if we did that, and then someone had this URL: Any generic regex that matches your chrome-extension URL would have to also match that How could we know that this URL should have been taken to be as absolute instead: We couldn't. Right now, "absolute" requires something like Unless, and this is the part I'm dubious on, the regex only allowed a specific safe-list of scheme names, not just a How would we decide which scheme names should be explicitly listed? What happens if someone later comes along and says, "hey, my page is running under this Seems like a future maintenance nightmare, that ensures we have to keep updating LABjs rather than keeping it stable and unchanging as much as possible, and that we're asking for breakages. Not sure what a good general solution to this would be. My initial reaction is similar to what I said in #93, which is that this is a case where you should tweak LABjs for your own needs, since it's a pretty niche use-case, and the pain (of this one specifically) is disproportionately high. |
OTOH, perhaps the more appropriate logic is, if the current page is a scheme type, you couldn't do an absolute regex that was also a scheme (unless it was an exact match). You'd have to do a normal web URL like "http://", or otherwise the URL could be reasonably assumed to definitely be relative to the scheme URI. Even if we could construct such special case logic, I'm not sure if the niche'ness of this deserves to be elevated to the core code. Undecided for now. |
Thanks for the extremely long explanation! The URL is actually properly formatted |
Interesting. So the only thing missing is the allowance for |
Correct. The scheme was the problem. Additionally, as mentioned im my And since I tend to take these things seriously, the RFC also states that What I just now realize, is that the check should actually be case |
OK, this will be queued up to be fixed. But, just to set expectations, we're collecting several small niche things like this together before releasing again. As you can tell, I release quite rarely, as stability is one of the main features of this library. I would recommend for now that you patch your own copy of LABjs. I'll leave this issue open until such a time as it's appropriate to fix for that next release (probably 2.1). Thanks! |
fixed root_domain regex, fixes getify#98
Hello, |
@mttcr thanks! to clarify, you're talking about?
|
The first one. I have some raw source in a string that I convert into a blob, then I load it with LABjs.
|
thanks! |
Improved on LABjs by adding: -> conditional loading based on element availability -> test multiple elements with AND-like condition (eg: $O.test().test().js().wait()) -> test multiple elements with OR-like condition (eg: $O.test('this, that').js().wait()) -> custom javascript attributes -> leaner code (6.3kb minified, 2.9kb gzipped) -> replaces .no-js with .js on html element -> queue test via $O.queueTest() -> end() to enable explicit end of test chain without breaking the chain (unlike sandbox()) -> loading initial js (entry point js - like app.js) via data attribute with data-only -> adjusted absolute uri regex gotten from a LABjs issue (getify/LABjs#98) -> automatically load polyfill service (polyfill.io) via options -> inject script tags at end of head tag opposed to LABjs inject at top (getify/LABjs#115) -> conditional script execution of inline scripts via $O.test().script() -> add snippet in docs for lazy css since this library will not handle that -> support CommonJS (not tested)
Want to use $LAB inside a chrome-extension, however the regex that extracts the root_domain does not match the href
chrome-extension:mmjeglkkcnglojkjoomifokhekiiiioe/www/index.html
. According to RFC3986, Section 3.1:so the correct regex would be
/^[a-z][\w\+-\/]+\:\/\/\/?[^\/]+/
.The text was updated successfully, but these errors were encountered: