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

refactor(pluginutils): replace micromatch with more lightweight picomatch. #306

Merged
merged 4 commits into from
May 2, 2020
Merged

Conversation

p-kuen
Copy link
Contributor

@p-kuen p-kuen commented Apr 13, 2020

Rollup Plugin Name: pluginutils

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

This just replaces micromatch dependency with the picomatch package. picomatch is part of micromatch and the pluginutils-plugin only uses the matcher function which is the same as using picomatch directly (See https://github.com/micromatch/micromatch/blob/master/index.js#L104).
This reduces dependency size.

Also added custom types until picomatch adds types on it's own. (See micromatch/picomatch#53 (comment))

@p-kuen p-kuen changed the title feat(pluginutils): replace micromatch with more lightweight picomatch. refactor(pluginutils): replace micromatch with more lightweight picomatch. Apr 13, 2020
@shellscape
Copy link
Collaborator

Thanks for suggesting this. The one big hangup for me right now is that I don't want plugins to be in the business of maintaining types for dependencies. That's ripe for technical debt. If the picomatch folks don't respond over the next few days, I would suggest adding those types to https://github.com/DefinitelyTyped/DefinitelyTyped/. If they get published there or in picomatch itself, then I'm all about merging this.

@p-kuen
Copy link
Contributor Author

p-kuen commented Apr 13, 2020

Alright, I understand that. I'll add the types to DefinitelyTyped if they don't respond.

@shellscape
Copy link
Collaborator

@Patcher56 We're still open to this change. Any word on the types situation?

@p-kuen
Copy link
Contributor Author

p-kuen commented Apr 22, 2020

Unfortunately no comment on the picomatch pull request. I am very busy at work rn, but will add the types to DefinitelyTyped on weekend. After that we can we remove the type declarations from this PR.

Is this ok for you?

Edit: Just opened a PR to DefinitelyTyped. DefinitelyTyped/DefinitelyTyped#44221

@p-kuen
Copy link
Contributor Author

p-kuen commented Apr 28, 2020

I just replaced the picomatch.d.ts file with the newly created @types/picomatch package.

@shellscape
Copy link
Collaborator

shellscape commented Apr 28, 2020

Thanks! I'll take care of that lock file mismatch for you a little later today (that's my plan anyhow) and we'll merge this.

@shellscape shellscape merged commit 70780a0 into rollup:master May 2, 2020
LarsDenBakker pushed a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020
* Replace micromatch with more lightweight picomatch.
This reduces dependency size.

* pluginutils: replace picomatch types with @types/picomatch.

* chore: update pnpm lock

Co-authored-by: shellscape <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants