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

deps: vendor minimatch #47486

Closed
wants to merge 2 commits into from
Closed

deps: vendor minimatch #47486

wants to merge 2 commits into from

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Apr 8, 2023

Refs: #40731
this PR includes two commits:
1the first commit adds minimatch as a dependency,
the second commit which is not really ready - is here to demonstrate minimatch usage, exposing some functionality on the node:path module.

once the first commit lands, we can expose globing functionality on node:path, node:fs and use it on node --test and node --watch
I wonder what would be a good way to maintain docs for such exposed functionality, the best example of existing dependency I can think of is undici, but that follows a SPEC

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions
  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. labels Apr 8, 2023
@MoLow MoLow force-pushed the vendor-minimatch branch from ec4c561 to ba1ac9d Compare April 8, 2023 21:32
@cjihrig
Copy link
Contributor

cjihrig commented Apr 8, 2023

Is there a plan for using core's error system and primordials?

@anonrig
Copy link
Member

anonrig commented Apr 9, 2023

My vote is -1. I'm not in favor of exposing a functionality of a direct dependency that we have no control over and does not use primordials.

@MoLow
Copy link
Member Author

MoLow commented Apr 9, 2023

Is there a plan for using core's error system and primordials?

How can that be done?

I'm not in favor of exposing a functionality of a direct dependency that we have no control over and does not use primordials.

I agree that might be an issue. perhaps we only use minimatch for node --test and node --watch without exposing the functionality itself

@MoLow MoLow mentioned this pull request Apr 9, 2023
@benjamingr
Copy link
Member

My vote is -1. I'm not in favor of exposing a functionality of a direct dependency that we have no control over and does not use primordials.

Worth mentioning that minimatch's author, isaac used to be (10 years ago) Node's BDFL and expressed interest (many years later, in the npm days probably) in adding it to core. He's also been pretty unresponsive to pings in GitHub about it since.

So it's not like taking some random person's project without asking or anything like that.

(Just giving that context in case it was missing)

@MoLow
Copy link
Member Author

MoLow commented Apr 9, 2023

He's also been pretty unresponsive to pings in GitHub about it since.

@benjamingr He has commented two days ago: #40731 (comment)

@MoLow MoLow closed this Apr 9, 2023
@MoLow MoLow deleted the vendor-minimatch branch April 9, 2023 19:51
@isaacs
Copy link
Contributor

isaacs commented Apr 10, 2023

Worth mentioning that minimatch's author, isaac used to be (10 years ago) Node's BDFL and expressed interest (many years later, in the npm days probably) in adding it to core. He's also been pretty unresponsive to pings in GitHub about it since.

I'm still here, just busy with stuff. I get a lot of pings, hard to respond to them all :)

MoLow added a commit to MoLow/node that referenced this pull request Apr 21, 2023
PR-URL: nodejs#47499
Refs: nodejs#47490
Refs: nodejs#47486
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
targos pushed a commit that referenced this pull request May 2, 2023
PR-URL: #47499
Refs: #47490
Refs: #47486
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
PR-URL: #47499
Refs: #47490
Refs: #47486
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
PR-URL: nodejs#47499
Refs: nodejs#47490
Refs: nodejs#47486
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants