| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
|
this is a good change - instanceof should be avoided - but perhaps this should have a test using the vm module? |
Sorry, something went wrong.
@ljharb can you elaborate on what you have in mind? URL is not a primordial object, it's not available to VM Script instances: $ node -p "require('vm').runInContext('URL', require('vm').createContext({})) === URL"
evalmachine.<anonymous>:1
URL
^
ReferenceError: URL is not defined
at evalmachine.<anonymous>:1:1
at Script.runInContext (vm.js:141:18)
at Object.runInContext (vm.js:279:6)
at [eval]:1:15
at Script.runInThisContext (vm.js:131:18)
at Object.runInThisContext (vm.js:295:38)
at Object.<anonymous> ([eval]-wrapper:10:26)
at Module._compile (internal/modules/cjs/loader.js:1200:30)
at evalScript (internal/process/execution.js:98:25)
at internal/main/eval_string.js:23:3
$ node -p "require('vm').runInContext('URL', require('vm').createContext({ URL })) === URL"
trueWe'd need a whole new implementation of URL (in case of Electron, Blink provides one, Node.js another one) to test this behaviour. |
Sorry, something went wrong.
|
ahhh, hm. what's the implementation of isURLInstance doing that would be robust to that scenario? |
Sorry, something went wrong.
More info on that on #34622, I have actually very little knowledge myself on that topic 🙈 |
Sorry, something went wrong.
|
Looking at that implementation, that's not "is URL instance", that's "has truthy href and origin properties", which is more like "is URL-like". Is the intention to allow { href: 1, origin: 2 } as a valid URL-like thing? |
Sorry, something went wrong.
|
@ljharb I'd say it's probably OK, using such an object would fail anyway when converted to string. Hopefully the isURLInstance will be updated to get a finer test if the current implementation creates issues. |
Sorry, something went wrong.
|
I think this should be labeled Author ready if no one objects. |
Sorry, something went wrong.
|
In case anyone is curious as to what isURLInstance does… Lines 1420 to 1422 in 22c52aa |
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: #34951 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
PR-URL: #34951 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
PR-URL: nodejs#34951 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
PR-URL: nodejs#34951 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
| Back | FazBrowse Home | New Git URL |
Related to #34622, in case there are several WHATWG URL implementations available, isURLInstance would work better than instanceof URL. Might also be more efficient, but I haven't run any benchmarks :)
Checklist