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

module: block requiring test/reporters without scheme #47831

Merged
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

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

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

@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 :]

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.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label May 3, 2023
@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
targos pushed a commit that referenced this pull request May 12, 2023
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>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
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
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

test/reporters can be required