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

Don't use babel-loader's cacheDirectory for production #516

Merged
merged 1 commit into from
Mar 25, 2019

Conversation

Lyrkan
Copy link
Collaborator

@Lyrkan Lyrkan commented Feb 10, 2019

Currently cacheDirectory is always set to true in babel-loader's options.

This can be an issue because the cache identifier is only based on direct Babel options, but not on external configs such as .browserslistrc files or if a browserslist key was added to the package.json (see babel/babel-loader#690).

Disabling that cache entirely in Encore while waiting for a proper solution in babel or babel-loader would probably not be a good idea, but we could mitigate the problem by disabling it only for the prod environment.

Closes #514

@weaverryan
Copy link
Member

I'm worried that while this will make sure the production build is always "correct", it can cause a real difference between dev builds and production builds - e.g. you tweak the browserslist, check things on dev (nothing really changed due to caching), see that it looks good in some browser, then deploy.

Alternative fixes could be: (A) documentation or (B) [a little crazy] but storing the browserslist value somewhere and warning the user when it's changed.

@Lyrkan
Copy link
Collaborator Author

Lyrkan commented Mar 4, 2019

@weaverryan Thought about it during the week-end and I don't seen any good solution:

  • If we keep the cache enabled all the time (and document it): not only it causes the issue described in babel-loader cache is not invalidated when browserslist's config changes #514 (no changes on the developer's machine), but if you are also using a CI pipeline that resets the node_modules folder, the final production build will also be different from your dev environment.

  • If we only disable the cache in production mode (this PR): we end-up with the issue you're talking about

  • If we disable the cache all the time: The build times will be increased (not sure by how much) and some people's builds already take a lot of time to complete

  • If we try to store the browserslist value: not sure that would be that easy to do, we'd have to call the same browserslist than @babel/preset-env (other deps can have their own version) and hope the preset doesn't do any change to it internally. Also, it would only fix the browserslist case, but from what I understood this is a more generic issue that can happen with other external dependencies.

@weaverryan
Copy link
Member

Ok, let's do this then. It'll be nice when it's fixed upstream in Babel

@weaverryan weaverryan merged commit ae74298 into symfony:master Mar 25, 2019
weaverryan added a commit that referenced this pull request Mar 25, 2019
This PR was merged into the master branch.

Discussion
----------

Don't use babel-loader's cacheDirectory for production

Currently `cacheDirectory` is always set to `true` in `babel-loader`'s options.

This can be an issue because the cache identifier is only based on direct Babel options, but not on external configs such as `.browserslistrc` files or if a `browserslist` key was added to the `package.json` (see babel/babel-loader#690).

Disabling that cache entirely in Encore while waiting for a proper solution in `babel` or `babel-loader` would probably not be a good idea, but we could mitigate the problem by disabling it only for the prod environment.

Closes #514

Commits
-------

ae74298 Don't use babel-loader's cacheDirectory for production
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.

None yet

2 participants