Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bootstrap: use internal.url instead of url #49649

Closed

Conversation

LiviaMedeiros
Copy link
Contributor

Refs: #49590 (comment)

Not sure if it might break any binary compatibility that would cause high semverness, or cause any significant regression.

cc @nodejs/process

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 14, 2023
@aduh95 aduh95 requested a review from joyeecheung September 14, 2023 11:20
@joyeecheung
Copy link
Member

joyeecheung commented Sep 14, 2023

I think we should keep it. The reason why it's here is to include url in the snapshot so that when users require('url') it is faster.

@LiviaMedeiros
Copy link
Contributor Author

Is there performance difference between importing it unconditionally here, and explicit import in user-provided snapshot code?
I'm curious, why we should unconditionally speed up url but not any other popular module.

@aduh95
Copy link
Contributor

aduh95 commented Sep 14, 2023

I'm curious, why we should unconditionally speed up url but not any other popular module.

What do you mean "popular module"? I think the goal is to bootstrap all the builtin modules so they're in the snapshot, unfortunately not all of them are snapshotable.

@LiviaMedeiros
Copy link
Contributor Author

Ah, I was under impression from

// Needed by the module loader and generally needed everywhere.
that we only pre-require modules that are necessary for loading process that happens in 100% cases.

If we want to include all possible builtins in snapshot, it indeed must stay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants