GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — esm: avoid try/catch when validating urls by anonrig · Pull Request #47541 · nodejs/node

Conversation

@anonrig

@anonrig anonrig commented Apr 13, 2023

Copy link
Copy Markdown
Member

Due to URL.canParse, we can avoid try/catch block and have faster validation. The previous implementation was not performant due to:

  1. Unnecessary string serialization - We don't need href, origin etc. for validating if a URL is valid or not.
  2. URL.canParse can be written with V8 Fast API - enabling more performance out of this pull request.
  3. URL.canParse does not return anything except a boolean.

cc @nodejs/url

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Apr 13, 2023
aduh95
aduh95 reviewed Apr 13, 2023
View reviewed changes

@aduh95 aduh95 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that URL.canParse is user-mutable, should we use const { canParse } = URL or something like that? Or even better, can we import it primordial-style from ada directly?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ljharb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@anonrig

anonrig commented Apr 13, 2023

Copy link
Copy Markdown
Member Author

Given that URL.canParse is user-mutable, should we use const { canParse } = URL or something like that? Or even better, can we import it primordial-style from ada directly?

@aduh95 I like where this is going, but I couldn't visualize it. Can you elaborate on how we can import it primordial-style? We can always do const { canParse } = internalBinding('url'); if that's what you are recommending?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

aduh95 commented Apr 13, 2023

Copy link
Copy Markdown
Contributor

Yes, I mean something that's available only internally and cannot be mutated at runtime. const { canParse } = internalBinding('url') is close enough, let's roll with this :)
ljharb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
anonrig force-pushed the improve-esm-performance branch from 5375959 to 3a57762 Compare April 13, 2023 15:52
@anonrig
anonrig requested a review from aduh95 April 13, 2023 15:53
aduh95
aduh95 approved these changes Apr 13, 2023
View reviewed changes
@anonrig anonrig added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Apr 13, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 13, 2023
@nodejs-github-bot

nodejs-github-bot commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51204/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig mentioned this pull request Apr 13, 2023
url: handle URL.canParse without base parameter #47547
Merged
@ljharb

ljharb commented Apr 13, 2023

Copy link
Copy Markdown
Member

This is way better/cleaner :-) but does it prevent backporting this commit, since older release lines may not have URL.canParse?
anonrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig

anonrig commented Apr 13, 2023

Copy link
Copy Markdown
Member Author

This is way better/cleaner :-) but does it prevent backporting this commit, since older release lines may not have URL.canParse?

It might make it harder, due to conflicts, but I don't think it will block backporting any future changes to ESM.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ljharb

ljharb commented Apr 13, 2023

Copy link
Copy Markdown
Member

oh sure, i just meant, should this PR have any "do not backport" labels, or link to the canParse PR as a prereq?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig

anonrig commented Apr 13, 2023

Copy link
Copy Markdown
Member Author

oh sure, i just meant, should this PR have any "do not backport" labels, or link to the canParse PR as a prereq?

I'm not quite sure. @nodejs/releasers what do you recommend?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JakobJingleheimer
JakobJingleheimer approved these changes Apr 13, 2023
View reviewed changes

@JakobJingleheimer JakobJingleheimer left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet, thanks! I was just suggesting this change last night in Slack 😁

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lpinca
lpinca approved these changes Apr 13, 2023
View reviewed changes
@aduh95

aduh95 commented Apr 13, 2023

Copy link
Copy Markdown
Contributor

oh sure, i just meant, should this PR have any "do not backport" labels, or link to the canParse PR as a prereq?

canParse was added in #47179, and there's no labels that says the backport is blocked, so no action should be necessary here I think. PRs are backported in the order they landed on Current release lines, precisely because it's often the case that a PR builds on top of another one.
ljharb reacted with thumbs up emoji ljharb reacted with rocket emoji
All reactions
  • 👍 1 reaction
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex

mscdex commented Apr 13, 2023

Copy link
Copy Markdown
Contributor

2. URL.canParse can be written with V8 Fast API - enabling more performance out of this pull request.

I'm confused by this. The C++ API still says this:

node/src/node_url.cc

Lines 116 to 117 in 4afb25c

// TODO(@anonrig): Add V8 Fast API for CanParse method
void BindingData::CanParse(const FunctionCallbackInfo<Value>& args) {

Or did you actually mean this PR does not improve performance until the TODO is resolved?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig

anonrig commented Apr 13, 2023

Copy link
Copy Markdown
Member Author

Or did you actually mean this PR does not improve performance until the TODO is resolved?

I was giving some context to this particular change. It is definitely faster than the current implementation. Additionally, @KhafraDev just opened a pull request for adding v8 fast API to canParse #47552
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ljharb

ljharb commented Apr 13, 2023

Copy link
Copy Markdown
Member

@aduh95 perfect, thanks for clarifying and linking :-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
esm: avoid try/catch when validating urls
05742e9
@anonrig
anonrig force-pushed the improve-esm-performance branch from 3a57762 to 05742e9 Compare April 14, 2023 13:45
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 14, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 14, 2023
@nodejs-github-bot

nodejs-github-bot commented Apr 14, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51262/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Apr 14, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51268/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
anonrig requested review from JakobJingleheimer, aduh95 and lpinca April 14, 2023 22:24
@anonrig

anonrig commented Apr 14, 2023

Copy link
Copy Markdown
Member Author

Would someone re-review this pull request? It's required due to force-push.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Qard
Qard approved these changes Apr 14, 2023
View reviewed changes
@anonrig anonrig added the commit-queue PRs queued for automated landing through the Commit Queue. label Apr 14, 2023
@github-actions github-actions Bot mentioned this pull request Apr 15, 2023
CI Reliability 2023-04-15 nodejs/reliability#543
Open
33 tasks
JakobJingleheimer
JakobJingleheimer approved these changes Apr 15, 2023
View reviewed changes
Comment thread lib/internal/modules/esm/hooks.js
} = require('internal/errors').codes;
const { exitCodes: { kUnfinishedTopLevelAwait } } = internalBinding('errors');
const { URL } = require('internal/url');
const { canParse: urlCanParse } = internalBinding('url');

@JakobJingleheimer JakobJingleheimer Apr 15, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this sounds a little Yoda-esque

Suggested change
const { canParse: urlCanParse } = internalBinding('url');
const { canParse: canParseURL } = internalBinding('url');

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aduh95 aduh95 Apr 15, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is better, isURLString might be more suited. URLCanParse has the upside of being consistent with how primordials are named, which is nice imho

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JakobJingleheimer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@anonrig anonrig Apr 15, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to change this to URLCanParse too but I'm too demotivated by having to run Node.js CI multiple times because of the flaky tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JakobJingleheimer reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Apr 15, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 17570c0 into nodejs:main Apr 15, 2023
@nodejs-github-bot

nodejs-github-bot commented Apr 15, 2023

Copy link
Copy Markdown
Collaborator

Landed in 17570c0
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Apr 18, 2023
CI Reliability 2023-04-18 nodejs/reliability#544
Open
35 tasks
targos pushed a commit that referenced this pull request May 2, 2023
@anonrig @targos
esm: avoid try/catch when validating urls
889add6 
PR-URL: #47541
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@targos targos mentioned this pull request May 2, 2023
v20.1.0 release proposal #47820
Merged
@danielleadams danielleadams added the backport-requested-v18.x label Jul 3, 2023
@danielleadams danielleadams mentioned this pull request Jul 3, 2023
esm: rename URLCanParse to be consistent #47668
Merged
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
@anonrig @targos
esm: avoid try/catch when validating urls
7c878ef 
PR-URL: nodejs#47541
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@targos targos mentioned this pull request Nov 11, 2023
[v18.x] Backport most ESM and customization hook changes #50669
Closed
@targos targos added backported-to-v18.x and removed backport-requested-v18.x labels Nov 23, 2023
targos pushed a commit that referenced this pull request Nov 23, 2023
@anonrig @targos
esm: avoid try/catch when validating urls
2a528b7 
PR-URL: #47541
Backport-PR-URL: #50669
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@targos targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
esm: avoid try/catch when validating urls
dbd86ae 
PR-URL: nodejs/node#47541
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
esm: avoid try/catch when validating urls
9c2429d 
PR-URL: nodejs/node#47541
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Qard Qard Qard approved these changes
@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes
@lpinca lpinca Awaiting requested review from lpinca
@aduh95 aduh95 Awaiting requested review from aduh95

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@anonrig @nodejs-github-bot @aduh95 @ljharb @mscdex @Qard @lpinca @JakobJingleheimer @targos @danielleadams
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.