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

Misc reorganizing and prep for ignore/only refactoring #5467

Merged
merged 7 commits into from
Mar 17, 2017

Conversation

loganfsmyth
Copy link
Member

@loganfsmyth loganfsmyth commented Mar 14, 2017

Q A
Patch: Bug Fix? N
Major: Breaking Change? Y, barely
Minor: New Feature? N
Deprecations?
Spec Compliancy?
Tests Added/Pass?
Fixed Tickets Fixes #1, Fixes #2
License MIT
Doc PR
Dependency Changes

Alongside the refactoring, this removes opts.basename used by some plugins in favor of using opts.filename directly and generating the basename in the few plugins it was used by.

EDIT:
Oh and a second breaking change. Previously if you specified only then ignore would never be processed. So the logic was

// is compilable?
if (opts.only) return matches(opts.only, filename);
if (opts.ignore) return !matches(opts.ignore, filename);
return true;

meaning that only: ['foo.js'], ignore: ['.js'] would compile a file named foo.js even though it is being ignored.

Now it is

// is compilable?
return matches(opts.only, filename) && !matches(opts.ignore, filename);

@loganfsmyth loganfsmyth added the PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release label Mar 14, 2017
@mention-bot
Copy link

@loganfsmyth, thanks for your PR! By analyzing the history of the files in this pull request, we identified @existentialism, @jamestalmage and @hzoo to be potential reviewers.

@codecov
Copy link

codecov bot commented Mar 14, 2017

Codecov Report

Merging #5467 into 7.0 will increase coverage by 0.19%.
The diff coverage is 85.54%.

@@            Coverage Diff            @@
##              7.0   #5467      +/-   ##
=========================================
+ Coverage   85.31%   85.5%   +0.19%     
=========================================
  Files         202     201       -1     
  Lines        9526    9510      -16     
  Branches     2702    2695       -7     
=========================================
+ Hits         8127    8132       +5     
+ Misses        907     887      -20     
+ Partials      492     491       -1
Impacted Files Coverage Δ
packages/babel-core/src/index.js 100% <ø> (+41.17%)
...ckages/babel-core/src/transformation/file/index.js 84.15% <100%> (-0.85%)
...l-plugin-transform-react-display-name/src/index.js 88.37% <100%> (ø)
...bel-plugin-transform-react-jsx-source/src/index.js 80% <100%> (+2.22%)
.../src/transformation/file/options/option-manager.js 81.1% <100%> (+1.97%)
...l-plugin-transform-es2015-modules-umd/src/index.js 82.45% <100%> (+0.31%)
packages/babel-core/src/transformation/pipeline.js 68.08% <71.42%> (+4.92%)
.../transformation/file/options/build-config-chain.js 93.87% <84.21%> (ø)
packages/babel-core/src/util.js 94.91% <90%> (-1.39%)
...bel-plugin-transform-es2015-classes/src/vanilla.js 90.59% <0%> (+0.85%)
... and 4 more

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 2642c2c...bf4664b. Read the comment docs.

if (opts.babelrc !== false) {
builder.findConfigs(filename);
}

builder.mergeConfig({
Copy link
Member Author

Choose a reason for hiding this comment

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

By swapping the order of these, we'll be able to bail out based on ignore and only match failure, before having to search for the .babelrc which will be a big performance win.

Copy link
Member

Choose a reason for hiding this comment

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

That's really good 🚀

}

opts.filename = filename;
opts = new OptionManager().init(opts);
Copy link
Member Author

Choose a reason for hiding this comment

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

Pulled out OptionManager#init calls into these so that we won't have to bother reading the file content from disk if the filename fails the ignore/only check.

@@ -111,20 +126,22 @@ export function booleanify(val: any): boolean | any {

export function shouldIgnore(
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes to this function make it required that you both match only and don't match ignore. With the old logic of only using only when present meant that we needed to aggregate the whole config tree before processing only and ignore. Like say you had

babel.transform(..., {ignore: "**/*.jsx"});

but your .babelrc was

{ 
  only: ['**/*.jsx'],
}

in v6 the logic would compile all .jsx files because it would pass only, and we're required to read the .babelrc of every file even if they won't match the ignore pattern passed to .transform.

@@ -84,16 +84,6 @@ describe("buildConfigChain", function () {
const expected = [
{
options: {
Copy link
Member Author

Choose a reason for hiding this comment

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

This diff isn't great, but what moved here was the .babelignore ordering. But since we just append ignore values together, this change in ordering should essentially be a noop.

const fileName = state.file.log.filename !== "unknown"
? state.file.log.filename
: null;
const fileName = state.file.opts.filename;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why this code was written this way anyway.

Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

phew did it. I think we should think about some more tests thou

@loganfsmyth loganfsmyth merged commit b6194a8 into babel:7.0 Mar 17, 2017
@loganfsmyth loganfsmyth deleted the clean-options branch March 17, 2017 03:25
@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 PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants