FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: use isURLInstance instead of instanceof by aduh95 · Pull Request #34951 · nodejs/node · GitHub

/ node Public

module: use isURLInstance instead of instanceof - #34951

Closed
aduh95 wants to merge 1 commit into
nodejs:masterfrom
aduh95:use-isurlinstance-createrequire
Closed

module: use isURLInstance instead of instanceof#34951
aduh95 wants to merge 1 commit into
nodejs:masterfrom
aduh95:use-isurlinstance-createrequire

Conversation

aduh95 commented Aug 27, 2020
edited
Loading

Copy link
Copy Markdown
Contributor

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
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules

ljharb commented Aug 27, 2020

Copy link
Copy Markdown
Member

this is a good change - instanceof should be avoided - but perhaps this should have a test using the vm module?

aduh95 commented Aug 28, 2020

Copy link
Copy Markdown
Contributor Author

perhaps this should have a test using the vm module?

@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"
true

We'd need a whole new implementation of URL (in case of Electron, Blink provides one, Node.js another one) to test this behaviour.

ljharb commented Aug 28, 2020

Copy link
Copy Markdown
Member

ahhh, hm. what's the implementation of isURLInstance doing that would be robust to that scenario?

aduh95 commented Aug 28, 2020

Copy link
Copy Markdown
Contributor Author

ahhh, hm. what's the implementation of isURLInstance doing that would be robust to that scenario?

More info on that on #34622, I have actually very little knowledge myself on that topic 🙈

ljharb commented Aug 28, 2020

Copy link
Copy Markdown
Member

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?

aduh95 commented Aug 28, 2020

Copy link
Copy Markdown
Contributor Author

@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.

aduh95 commented Sep 7, 2020

Copy link
Copy Markdown
Contributor Author

I think this should be labeled Author ready if no one objects.

Copy link
Copy Markdown
Contributor

In case anyone is curious as to what isURLInstance does…

node/lib/internal/url.js

Lines 1420 to 1422 in 22c52aa

function isURLInstance(fileURLOrPath) {
return fileURLOrPath != null && fileURLOrPath.href && fileURLOrPath.origin;
}

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2020

Copy link
Copy Markdown
Collaborator

github-actions Bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 15, 2020

Copy link
Copy Markdown
Contributor

Landed in d24eecd

github-actions Bot closed this Sep 15, 2020
nodejs-github-bot pushed a commit that referenced this pull request Sep 15, 2020
PR-URL: #34951
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
ruyadorno pushed a commit that referenced this pull request Sep 21, 2020
PR-URL: #34951
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
ruyadorno mentioned this pull request Sep 21, 2020
4 tasks
guybedford pushed a commit to guybedford/node that referenced this pull request Sep 28, 2020
PR-URL: nodejs#34951
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
PR-URL: #34951
Backport-PR-URL: #35385
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
codebytere mentioned this pull request Oct 4, 2020
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#34951
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
aduh95 deleted the use-isurlinstance-createrequire branch July 16, 2022 16:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. module Issues and PRs related to the module subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL