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

feat(v2): env variable TERSER_PARALLEL to customize TerserPlugin.parallel #3497

Merged
merged 9 commits into from
Sep 29, 2020

Conversation

aeneasr
Copy link
Contributor

@aeneasr aeneasr commented Sep 29, 2020

Motivation

Do not run Terser in parallel when using CircleCI or similar CI environments
to avoid "Error: Call retries were exceeded" errors. For more information
see https://github.com/webpack-contrib/terser-webpack-plugin#parallel

CI=true is true for:

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

None - hard to test because certain conditions need to be met and because it's not possible to replicate those easily without a published package.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 29, 2020
@aeneasr
Copy link
Contributor Author

aeneasr commented Sep 29, 2020

If this could make it into the next release, that would be great as this is blocking all of our documentation builds (and therefore releases) for e.g. ORY Hydra which has a patch in pipeline for fixing a CVE! Thank you!

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Sep 29, 2020

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit e42f632

https://deploy-preview-3497--docusaurus-2.netlify.app

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2020

Hi,

I find this funny because Docusaurus is used on 2 sites:

  • ory/hydra
  • facebook/hydra (managed by @omry 😅 )

Afaik this option does not affect all CI environments.
Nobody complained so far about this option, while Docusaurus on Netlify for example (which does set CI=true)

I'm willing to provide you an option to disable it for your site only, without changing behavior for other sites that work fine with parallel: true, that seems to work fine with other CI options anyway.

So I'd rather have you run a command like TERSER_PARALLEL=false yarn build or TERSER_PARALLEL=2 yarn build, but I think it makes sense to keep it true by default on CIs.

If we don't do this, users will report build perf regressions after upgrading.

Ideally, I'd even prefer a dedicated build option, like yarn build --parallel false, as it's more explicit and self-documented by the cli help:

image

We can get this released in next release if PR is ready soon, likely tomorrow (cc @lex111 ). In the meantime you can use patch-package to apply the change locally

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2020

Is this a problem that affects ALL circle CI sites?
Can changing parallel true to false be a regression for any circle CI site?

A less flexible but easier solution could also be to use something like:

const terserParallel = !process.env.CIRCLECI === "true"

@aeneasr
Copy link
Contributor Author

aeneasr commented Sep 29, 2020

So I'd rather have you run a command like TERSER_PARALLEL=false yarn build or TERSER_PARALLEL=2 yarn build, but I think it makes sense to keep it true by default on CIs.

Sounds good - should I update the PR accordingly?

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2020

Yes, if the PR is good I'll merge it for next release (likely tomorrow so don't wait :p)

@aeneasr
Copy link
Contributor Author

aeneasr commented Sep 29, 2020

Sorry for the many commits - you can squash merge it once it passes :)

@aeneasr
Copy link
Contributor Author

aeneasr commented Sep 29, 2020

Ok, sorry for the many commits. The changes were so minimal I thought I could do them in the web editor but well it turned out my guessing game with eslint/prettier didn't work out so well. Checked it out locally so everything should be good to go IMO

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2020

Thanks, LGTM.

Let's keep it this way for now, and add doc and a cli option later if the need for such option becomes important for more users.

We don't care about commits as all PRs are squash merged anyway ;)

@aeneasr
Copy link
Contributor Author

aeneasr commented Sep 29, 2020

Nice, thanks for the quick turnaround!

@slorber slorber changed the title fix(v2): disable terser parallel on CIs feat(v2): env variable TERSER_PARALLEL to customize TerserPlugin.parallel Sep 29, 2020
@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Sep 29, 2020
@slorber slorber merged commit db051ee into facebook:master Sep 29, 2020
@omry
Copy link

omry commented Sep 29, 2020

Hi,

I find this funny because Docusaurus is used on 2 sites:

  • ory/hydra
  • facebook/hydra (managed by @omry 😅 )

Yup, it is a bit funny.
Docusaurus is welcoming any Hydra, regardless of color and affiliation.

@lex111 lex111 added this to the v2.0.0-alpha.65 milestone Sep 29, 2020
@aeneasr
Copy link
Contributor Author

aeneasr commented Oct 2, 2020

No pressure, just checking in if the release is to be expected this week - otherwise I'll probably fixup our CI pipeline to patch the package in the meanwhile!

@aeneasr aeneasr deleted the patch-2 branch October 2, 2020 08:40
aeneasr added a commit to ory/hydra that referenced this pull request Oct 2, 2020
@slorber
Copy link
Collaborator

slorber commented Oct 2, 2020

Hi, sorry we delayed the release but it should be in the next hours ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants