Skip to content

module: reduce url invocations in esm/load.js#48337

Merged
nodejs-github-bot merged 2 commits intonodejs:mainfrom
anonrig:improve-esm-loadjs
Jun 7, 2023
Merged

module: reduce url invocations in esm/load.js#48337
nodejs-github-bot merged 2 commits intonodejs:mainfrom
anonrig:improve-esm-loadjs

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Jun 5, 2023 •
edited
Loading

Reduces unnecessary new URL call, and removes unnecessary invokes to url.protocol

Ref: nodejs/performance#92

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 5, 2023

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@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 Jun 5, 2023
@anonrig anonrig force-pushed the improve-esm-loadjs branch from cadc77b to 724ee2e Compare June 5, 2023 01:13
@anonrig anonrig force-pushed the improve-esm-loadjs branch from 724ee2e to bd035a6 Compare June 5, 2023 01:31
aduh95
aduh95 approved these changes Jun 5, 2023
View reviewed changes
@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 5, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 5, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 5, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/52115/

GeoffreyBooth
GeoffreyBooth approved these changes Jun 5, 2023
View reviewed changes
JakobJingleheimer
JakobJingleheimer approved these changes Jun 6, 2023
View reviewed changes
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

I think we had talked about something similar previously, where I suggested the load hook be passed a URL instance (which we already have internally), but for some reason that I don't recall that was not good.

I think whatever that reason (something about mutability?) was, it doesn't apply here because this is internal.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 7, 2023
@nodejs-github-bot nodejs-github-bot merged commit 9d936fa into nodejs:main Jun 7, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 7, 2023

Landed in 9d936fa

RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@anonrig @RafaelGSS
module: reduce url invocations in esm/load.js
4ec2d92 
PR-URL: #48337
Refs: nodejs/performance#92
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig @Ceres6
module: reduce url invocations in esm/load.js
cb3c3af 
PR-URL: nodejs#48337
Refs: nodejs/performance#92
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig @Ceres6
module: reduce url invocations in esm/load.js
7673e40 
PR-URL: nodejs#48337
Refs: nodejs/performance#92
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
ruyadorno pushed a commit that referenced this pull request Aug 29, 2023
@anonrig @ruyadorno
module: reduce url invocations in esm/load.js
bf1525c 
PR-URL: #48337
Refs: nodejs/performance#92
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
@ruyadorno ruyadorno mentioned this pull request Aug 29, 2023
Merged
codebytere added a commit to electron/electron that referenced this pull request Sep 19, 2023
@codebytere
module: reduce url invocations in esm/load.js
2e19f7f 
nodejs/node#48337
codebytere added a commit to electron/electron that referenced this pull request Sep 19, 2023
@codebytere
module: reduce url invocations in esm/load.js
c89b97f 
nodejs/node#48337
codebytere added a commit to electron/electron that referenced this pull request Sep 20, 2023
@codebytere
module: reduce url invocations in esm/load.js
5c52801 
nodejs/node#48337
codebytere added a commit to electron/electron that referenced this pull request Sep 20, 2023
@codebytere
module: reduce url invocations in esm/load.js
f81d844 
nodejs/node#48337
codebytere added a commit to electron/electron that referenced this pull request Sep 20, 2023
@codebytere
module: reduce url invocations in esm/load.js
7c478f5 
nodejs/node#48337
jkleinsc pushed a commit to electron/electron that referenced this pull request Sep 20, 2023
@electron-roller @codebytere
chore: bump node to v18.18.0 (main) (#39915)
1766511 
* chore: bump node in DEPS to v18.18.0

* child_process: harden against prototype pollution

nodejs/node#48726

* deps: upgrade to libuv 1.46.0

nodejs/node#49591

* module: reduce url invocations in esm/load.js

nodejs/node#48337

* Revert "test: remove test-crypto-keygen flaky designation"

nodejs/node#48652

* fix: FTBTFS in ada dep

ada-url/ada#464
ada-url/idna#31

* fix: force_colors snapshot line number

* chore: fixup patch indices

* chore: update filenames.json

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
@electron-roller @codebytere
chore: bump node to v18.18.0 (main) (electron#39915)
a848ba1 
* chore: bump node in DEPS to v18.18.0

* child_process: harden against prototype pollution

nodejs/node#48726

* deps: upgrade to libuv 1.46.0

nodejs/node#49591

* module: reduce url invocations in esm/load.js

nodejs/node#48337

* Revert "test: remove test-crypto-keygen flaky designation"

nodejs/node#48652

* fix: FTBTFS in ada dep

ada-url/ada#464
ada-url/idna#31

* fix: force_colors snapshot line number

* chore: fixup patch indices

* chore: update filenames.json

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
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

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@lpinca lpinca lpinca approved these changes

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

7 participants

@anonrig @nodejs-github-bot @Qard @GeoffreyBooth @lpinca @JakobJingleheimer @aduh95