Skip to content

module: block requiring test/reporters without scheme#47831

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 2 commits into from
May 5, 2023
Merged

module: block requiring test/reporters without scheme #47831

nodejs-github-bot merged 2 commits into from
May 5, 2023

Conversation

@MoLow
Copy link
Member

@MoLowMoLow commented May 3, 2023

Fixes: #47828

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 3, 2023

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
  • @nodejs/startup

@nodejs-github-botnodejs-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 May 3, 2023
@VoltrexKeyva
Copy link
Member

VoltrexKeyva commented May 3, 2023•
edited
Loading

s/shcema/scheme in commit message.

@targos
Copy link
Member

targos commented May 3, 2023

It should be scheme (or prefix), not schema.

@MoLowMoLowforce-pushed the fix-test-reporters-schema branch from 957ba10 to 92c05c2CompareMay 3, 2023 08:13
@panvapanva changed the title module: block requiring test/reporters without shcemamodule: block requiring test/reporters without schemeMay 3, 2023
@MoLowMoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 3, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 3, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 3, 2023

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

RaisinTen
RaisinTen approved these changes May 3, 2023
View reviewed changes
Copy link
Member

@RaisinTenRaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM but should we also add a test that requiring this without the scheme throws?

@MoLow
Copy link
MemberAuthor

MoLow commented May 3, 2023

@RaisinTen added a test, can you please re-approve?

@MoLowMoLow added the request-ci Add this label to start a Jenkins CI on a PR. label May 3, 2023
RaisinTen
RaisinTen approved these changes May 3, 2023
View reviewed changes
Copy link
Member

@RaisinTenRaisinTen left a comment

Choose a reason for hiding this comment

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

Thanks, still LGTM

@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 3, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 3, 2023

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

@ljharb
Copy link
Member

ljharb commented May 3, 2023

Is this not a breaking change?

@MoLow
Copy link
MemberAuthor

MoLow commented May 3, 2023

Is this not a breaking change?

no, it is a bugfix. according to documentation:

This module is only available under the node: scheme. The following will not work:

@ljharb
Copy link
Member

ljharb commented May 3, 2023

I get that you can call it a patch, but it will be a breakage for anyone using is-core-module, including anyone using resolve.

@targos
Copy link
Member

targos commented May 3, 2023

Anyone using it? Really? Don't they have to pass test/reporters to it?

@ljharb
Copy link
Member

ljharb commented May 3, 2023

What i mean is, tooling based on resolve will consider the schemeless one a core module. So yes, they’d have to be requiring it, but it’s a pretty big assumption that nobody’s doing that already.

@benjamingr
Copy link
Member

benjamingr commented May 3, 2023

What i mean is, tooling based on resolve will consider the schemeless one a core module

So I checked is-core-module and found inspect-js/is-core-module@9d5341a from 3 weeks ago so it doesn't look like a lot of time passed and the breakage seems reasonable in this case to not require a major.

On a different note next time you see this sort of bug or oddity please report it :]

aduh95
aduh95 approved these changes May 3, 2023
View reviewed changes
Copy link
Contributor

@aduh95aduh95 left a comment

Choose a reason for hiding this comment

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

LGTM, I agree it makes sense to land this as a patch

@ljharb
Copy link
Member

ljharb commented May 3, 2023

@benjamingr the bug or oddity imo is that test requires the scheme, and the only rationale i was aware of for requiring it was that test might be something people are already using. test/reporters didn't seem like it had that rationale.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 3, 2023

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

@MoLowMoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label May 3, 2023
This was referenced May 4, 2023
Open
Open
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 5, 2023
@nodejs-github-botnodejs-github-bot merged commit d55b84b into nodejs:mainMay 5, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 5, 2023

Landed in d55b84b

@MoLowMoLow deleted the fix-test-reporters-schema branch May 5, 2023 08:46
This was referenced May 6, 2023
Open
Open
Open
Open
targos pushed a commit that referenced this pull request May 12, 2023
@MoLow@targos
module: block requiring test/reporters without scheme
739113f
PR-URL: #47831Fixes: #47828 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
@targostargos mentioned this pull request May 15, 2023
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@MoLow@danielleadams
module: block requiring test/reporters without scheme
8afdc5b
PR-URL: #47831Fixes: #47828 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
@MoLow
module: block requiring test/reporters without scheme
cf8845d
PR-URL: nodejs#47831Fixes: nodejs#47828 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@panvapanvapanva approved these changes

@benjamingrbenjamingrbenjamingr approved these changes

@anonriganonriganonrig approved these changes

@targostargostargos approved these changes

@JakobJingleheimerJakobJingleheimerJakobJingleheimer approved these changes

@aduh95aduh95aduh95 approved these changes

@RaisinTenRaisinTenRaisinTen 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.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test/reporters can be required

11 participants

@MoLow@nodejs-github-bot@VoltrexKeyva@targos@ljharb@benjamingr@panva@anonrig@JakobJingleheimer@aduh95@RaisinTen