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

Seperate version env cache files #5411

Merged
merged 2 commits into from
Mar 22, 2017

Conversation

pwmckenna
Copy link
Contributor

Q A
Patch: Bug Fix? ?
Major: Breaking Change? ?
Minor: New Feature? ?
Deprecations? N
Spec Compliancy? N
Tests Added/Pass? N
License MIT

Because cache entries are keyed on babel version and environment, there's no point in storing different combinations in the same file, because you'll never use the portions of the cache generated with different combinations. For instance, if you use BABEL_ENV values 'node' and 'test', you'll have to load a cache that's twice as big as you'll possibly use.

@mention-bot
Copy link

@pwmckenna, thanks for your PR! By analyzing the history of the files in this pull request, we identified @xtuc, @jonathanong and @sindresorhus to be potential reviewers.

@codecov
Copy link

codecov bot commented Mar 3, 2017

Codecov Report

Merging #5411 into 7.0 will decrease coverage by <.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##              7.0    #5411      +/-   ##
==========================================
- Coverage   85.34%   85.33%   -0.01%     
==========================================
  Files         201      201              
  Lines        9485     9486       +1     
  Branches     2689     2689              
==========================================
  Hits         8095     8095              
- Misses        897      898       +1     
  Partials      493      493
Impacted Files Coverage Δ
packages/babel-register/src/cache.js 76.19% <50%> (+1.19%) ⬆️
packages/babel-traverse/src/visitors.js 85.71% <0%> (-0.96%) ⬇️
packages/babel-traverse/src/path/context.js 85.34% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95c905c...4c008ad. Read the comment docs.


const FILENAME: string = process.env.BABEL_CACHE_PATH || path.join(homeOrTmp, ".babel.json");
const env = process.env.BABEL_ENV || process.env.NODE_ENV;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could set the default value here, like:

const env = process.env.BABEL_ENV || process.env.NODE_ENV || "";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I don't think that would save us the check below. If neither is set we could end up with something like .babel.6.x.x..json with the extra ....which might not be a problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that right. The default environment is development:

const env = process.env.BABEL_ENV || process.env.NODE_ENV || "development";

Copy link
Member

@xtuc xtuc Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made #5448 for that purpose. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks great! I'll keep an eye out and update this when it gets merged in. Thanks for that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR has been merged

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

@xtuc xtuc mentioned this pull request Mar 11, 2017
@pwmckenna pwmckenna force-pushed the seperate-version-env-cache-files branch from b1e59d6 to 1256d00 Compare March 16, 2017 20:08
Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR 👍

@pwmckenna pwmckenna force-pushed the seperate-version-env-cache-files branch from 1256d00 to 4c008ad Compare March 21, 2017 13:50
@hzoo hzoo merged commit b2c977a into babel:7.0 Mar 22, 2017
@hzoo hzoo added the PR: Polish 💅 A type of pull request used for our changelog categories label Mar 22, 2017
@pwmckenna pwmckenna deleted the seperate-version-env-cache-files branch March 22, 2017 22:36
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: register PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants