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

Consider using micromatch #1158

Closed
wants to merge 3 commits into from
Closed

Consider using micromatch #1158

wants to merge 3 commits into from

Conversation

doowb
Copy link
Contributor

@doowb doowb commented Mar 27, 2017

Hi,

This PR is doing two things:

  • Replace minimatch with micromatch.
  • Update the glob pattern for matching valid header files so only one pass is made.

When the glob pattern is updated to '*.{h,gypi}', micromatch will create one regexp pattern that will only do one pass over the file being checked. This differs from minimatch that creates 2 different regexp patterns and will check the file twice if the pattern doesn't match '*.h'.

Thanks for considering this.

@bnoordhuis
Copy link
Member

Thanks for the PR. The reason we use minimatch is because npm does, meaning we can share a copy with it and make the installer a little smaller.

I wouldn't object to removing minimatch altogether if size of the standalone install is your motivation for doing this. It's not like we are doing anything fancy with it; a simple regex probably suffices.

@doowb
Copy link
Contributor Author

doowb commented Mar 28, 2017

if size of the standalone install is your motivation for doing this

No, it was more that I use micromatch often and I recognized a quick optimization that may be useful since almost any time I do an npm install I see something using node-gyp.

a simple regex probably suffices

It probably does. I can update this PR with a regex change if you'd like. I know that micromatch expands the glob pattern into a more thorough regex than I would probably think of and there may be something additional done because of the matchBase: true option, but I'll look into it.

@doowb
Copy link
Contributor Author

doowb commented Jun 30, 2017

@bnoordhuis I updated this to just use path.extname since the patterns are just checking for the file extensions.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

👍

bnoordhuis pushed a commit that referenced this pull request Jun 8, 2018
PR-URL: #1158
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@bnoordhuis
Copy link
Member

Thanks Brian, landed in d70513a.

@bnoordhuis bnoordhuis closed this Jun 8, 2018
bnoordhuis pushed a commit that referenced this pull request Jun 8, 2018
PR-URL: #1158
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants