Skip to content

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