Skip to content

module: add prefix-only modules to module.builtinModules#56185

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 1 commit into from
Dec 14, 2024
Merged

module: add prefix-only modules to module.builtinModules#56185

nodejs-github-bot merged 1 commit into from
Dec 14, 2024

Conversation

@ljharb
Copy link
Member

@ljharbljharb commented Dec 8, 2024•
edited by aduh95
Loading

Fixes: #42785

I haven't added tests yet; I wanted to get some feedback first.

@ljharbljharb added the module Issues and PRs related to the module subsystem. label Dec 8, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 8, 2024

Review requested:

  • @nodejs/loaders

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 8, 2024
juanarbol
juanarbol reviewed Dec 9, 2024
View reviewed changes
juanarbol
juanarbol requested changes Dec 9, 2024
View reviewed changes
Copy link
Member

@juanarboljuanarbol left a comment

Choose a reason for hiding this comment

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

I don't have major complaints against this, thanks!

juanarbol
juanarbol reviewed Dec 9, 2024
View reviewed changes
@ljharbljharbforce-pushed the builtinPrefixOnlyModules branch from 60f1b3a to 31ed23cCompareDecember 9, 2024 06:46
@ljharbljharb requested a review from juanarbolDecember 9, 2024 06:46
aduh95
aduh95 reviewed Dec 9, 2024
View reviewed changes
aduh95
aduh95 reviewed Dec 9, 2024
View reviewed changes
juanarbol
juanarbol reviewed Dec 9, 2024
View reviewed changes
BridgeAR
BridgeAR reviewed Dec 9, 2024
View reviewed changes
Copy link
Member

@BridgeARBridgeAR left a comment

Choose a reason for hiding this comment

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

I would rather not add a separate API and just do what was mainly suggested in the issue: to add the missing entries with prefix to the list or just do: #42785 (comment)

@ljharb
Copy link
MemberAuthor

ljharb commented Dec 9, 2024•
edited
Loading

I can totally do that (the former; the latter doesn't match the state of the loader file) - while thinking about it I was concerned that might be a breaking change, because currently you can prepend node: to every item in the list and get a valid specifier, and the change would violate that - which would limit its backportability.

@BridgeAR
Copy link
Member

BridgeAR commented Dec 9, 2024

The prefix itself is still relatively new, so I guess programmatic addition is not yet common.
The semverness of the addition is debatable due to that as well. I would probably start with do-not-land-on-X labels and discuss backporting later.

@ljharb
Copy link
MemberAuthor

ljharb commented Dec 9, 2024

The prefix has been around since 2020, I believe - the thing that's new is prefix-only modules.

If that's the preferred path forward then I'll update this PR to do that, but it would be unfortunate imo if we weren't able to backport it.

@ljharbljharbforce-pushed the builtinPrefixOnlyModules branch from 31ed23c to 0d8ea8bCompareDecember 9, 2024 22:07
@ljharbljharb changed the title module: add module.builtinPrefixOnlyModulesmodule: add prefix-only modules to module.builtinModulesDec 9, 2024
@aduh95
Copy link
Contributor

aduh95 commented Dec 10, 2024

Is this still a draft or is it ready for reviews?

@ljharbljharb marked this pull request as ready for review December 10, 2024 00:41
@ljharbljharbforce-pushed the builtinPrefixOnlyModules branch from 0d8ea8b to 0dee8beCompareDecember 10, 2024 00:53
@codecov
Copy link

codecovbot commented Dec 10, 2024•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.52%. Comparing base (64cc3b8) to head (4383805).
Report is 1116 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #56185 +/- ## ========================================== - Coverage 88.54% 88.52% -0.02%  ========================================== Files 657 657 Lines 189970 189973 +3 Branches 36482 36481 -1 ========================================== - Hits 168200 168172 -28 - Misses 14979 14995 +16 - Partials 6791 6806 +15
Files with missing linesCoverage Δ
lib/internal/bootstrap/realm.js96.03% <100.00%> (-0.82%)⬇️
lib/internal/modules/cjs/loader.js98.11% <100.00%> (ø)
lib/repl.js94.95% <100.00%> (ø)

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ljharbljharb added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Dec 10, 2024
@ljharbljharbforce-pushed the builtinPrefixOnlyModules branch from 0dee8be to a806beeCompareDecember 10, 2024 20:05
@ljharbljharb deleted the builtinPrefixOnlyModules branch December 14, 2024 16:11
@aduh95aduh95 mentioned this pull request Dec 17, 2024
Merged
@aduh95
Copy link
Contributor

aduh95 commented Dec 17, 2024

FWIW this is breaking our own linter (#56284), so there might be more ecosystem breakage than CITGM shows us.

@legendecaslegendecas mentioned this pull request Dec 17, 2024
Closed
nodejs-github-bot pushed a commit that referenced this pull request Dec 17, 2024
@aduh95
tools: fix node: enforcement for docs
6012a4e
PR-URL: #56284 Refs: #56185 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
aduh95 pushed a commit that referenced this pull request Dec 18, 2024
@ljharb@aduh95
module: add prefix-only modules to module.builtinModules
a981886
Fixes#42785 PR-URL: #56185Fixes: #42785 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
aduh95 added a commit that referenced this pull request Dec 18, 2024
@aduh95
tools: fix node: enforcement for docs
88c4137
PR-URL: #56284 Refs: #56185 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
@github-actionsgithub-actionsbot mentioned this pull request Dec 18, 2024
Merged
@ljharbljharb mentioned this pull request Feb 15, 2025
Closed
@aduh95aduh95 mentioned this pull request Feb 16, 2025
Merged
@AriPerkkioAriPerkkio mentioned this pull request Feb 19, 2025
Merged
6 tasks
@boneskull
Copy link
Member

boneskull commented May 12, 2025

I note that this also breaks our linting

@boneskullboneskull mentioned this pull request May 12, 2025
Closed
@aduh95aduh95 added dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels May 12, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95aduh95aduh95 left review comments

@jasnelljasnelljasnell approved these changes

@BridgeARBridgeARBridgeAR approved these changes

@juanarboljuanarboljuanarbol approved these changes

@marco-ippolitomarco-ippolitomarco-ippolito approved these changes

@atlowChemiatlowChemiatlowChemi approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.dont-land-on-v20.xPRs that should not land on the v20.x-staging branch and should not be released in v20.x.dont-land-on-v22.xPRs that should not land on the v22.x-staging branch and should not be released in v22.x.moduleIssues and PRs related to the module subsystem.needs-ciPRs that need a full CI run.semver-minorPRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

no way to programmatically discover node:test

9 participants

@ljharb@nodejs-github-bot@BridgeAR@aduh95@boneskull@jasnell@juanarbol@marco-ippolito@atlowChemi