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

chore: vendor micromatch under @jest/micromatch #8046

Closed
wants to merge 6 commits into from
Closed

chore: vendor micromatch under @jest/micromatch #8046

wants to merge 6 commits into from

Conversation

wtgtybhertgeghgtwtg
Copy link
Contributor

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg commented Mar 4, 2019

Summary

Proposal for temporary measure as discussed in #8032 (use a rollup'd version of micromatch@3 until micromatch@4 becomes available to avoid require's). Probably needs a license note somewhere.

Test plan

Everything should work as it did.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

Browser build is failing because it can't find net. It works if I add net: 'empty' to scripts/browserBuild.js, but I don't know if you want to do that just for this.

@jeysal
Copy link
Contributor

jeysal commented Mar 4, 2019

Thanks for doing this, I'll profile the performance of this when I have some time this week so we can better decide if it's worth it.

@jeysal
Copy link
Contributor

jeysal commented Mar 5, 2019

jasmine2() total times in ms (very consistent within a few ms):
bundled (#8046): 750
micromatch3 (master): 1410
micromatch2 (#8032): 920

So the bundled micromatch 3 is even better than micromatch 2. We really seem to have a problem with loading many small modules, which could also come from our custom require impl / module registry, given that a normal Node require('micromatch') does not surface such extreme differences.
Anyone have ideas what the issues with our require performance could be? Would be cool to optimize that a little, either way we should probably keep bundling code that is loaded in the runtime at the back of our minds as a possible performance optimization.

@DylanVann
Copy link
Contributor

Way out of scope of fixing this regression, but it sounds like Jest may benefit from bundling in general. This article on performance of bundled vs. unbundled code is interesting: https://zeit.co/blog/ncc#faster-bootup

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

I'm against bundling dependencies for a good number of reasons. The only reason I put forward vendoring micromatch is because the require time problem has been an issue for almost a year and the alternatives are not any better.

@cozmy
Copy link

cozmy commented Mar 11, 2019

Hi! Any ideas when this will be merged?

@SimenB
Copy link
Member

SimenB commented Mar 11, 2019

The browser build is broken in this PR which needs to be fixed first.

That said, I'm more in favor of waiting for micromatch@4 to come out.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

It can be fixed by adding net: 'empty' to scripts/browserBuild.js, I'd just like some direction on whether or not I should do that.
I don't like the idea of bundling dependcies, but it's better than using a version with a CVE.

@SimenB
Copy link
Member

SimenB commented Mar 11, 2019

I'm more afraid about it being a breaking change to use v2 (more specifically, going from 2 to 3/4 later is breaking) than the CVE. If CVEs stops people's build pipelines with no way to ignore specific dependencies (in this case, it's a development time one), they should fix their pipeline.

That said, downgrading is, unless everybody strongly disagrees with me, not an option

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg marked this pull request as ready for review March 12, 2019 20:31
@wtgtybhertgeghgtwtg
Copy link
Contributor Author

Tests failing linting because of license check. I'm not sure how that'd work; I don't think vendored stuff is supposed to have a Facebook license.

@jeysal
Copy link
Contributor

jeysal commented Mar 12, 2019

@SimenB sooooo would you be fine with using this temporarily? In case micromatch 4 really does come out soon (and performs well) we don't really lose anything (this might not even be in a Jest release if it's really soon) but if it ends up taking longer we'll help people esp. with small projects a lot.

@scotthovestadt
Copy link
Contributor

@jeysal Can you provide instructions on how to benchmark it? I want to look into the root cause of the slowness & see if changes I made haven't already improved it.

@jeysal
Copy link
Contributor

jeysal commented Mar 21, 2019

We just had a call about this, a quick summary for documentation purposes:

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

Looks like micromatch@4 is out.

@jeysal
Copy link
Contributor

jeysal commented Apr 12, 2019

And it's probably really really tiny, compare v2, v3, v4

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants