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.

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

module: block requiring test/reporters without scheme #47831

Merged
merged 2 commits into from
May 5, 2023
Merged

module: block requiring test/reporters without scheme #47831

merged 2 commits into from
May 5, 2023

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented May 3, 2023

Fixes: #47828

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
  • @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 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.

@MoLow MoLow force-pushed the fix-test-reporters-schema branch from 957ba10 to 92c05c2 Compare May 3, 2023 08:13
@panva panva changed the title module: block requiring test/reporters without shcema module: block requiring test/reporters without scheme May 3, 2023
@MoLow MoLow 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-actions github-actions bot 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

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

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

@RaisinTen RaisinTen 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
Member Author

MoLow commented May 3, 2023

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

@MoLow MoLow 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
Contributor

@RaisinTen RaisinTen 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-actions github-actions bot 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

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
Member Author

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

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

@aduh95 aduh95 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

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

@MoLow MoLow 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-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 5, 2023
@nodejs-github-bot nodejs-github-bot merged commit d55b84b into nodejs:main May 5, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in d55b84b

@MoLow MoLow 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: #47831
Fixes: #47828
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
@targos targos 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: #47831
Fixes: #47828
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
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#47831
Fixes: nodejs#47828
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@aduh95 aduh95 aduh95 approved these changes

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@targos targos targos approved these changes

@anonrig anonrig anonrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

@panva panva panva 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. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test/reporters can be required