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

Make only/ignore relative to cwd/config file and move only/ignore checking all to core. #5487

Merged
merged 3 commits into from Mar 17, 2017

Conversation

Projects
None yet
5 participants
@loganfsmyth
Member

loganfsmyth commented Mar 17, 2017

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

Breaking changes:

  1. This PR makes ignore and only relative paths rather than arbitrary matching patterns, so configs for Babel 7 will likely need to be updated to match, if they used ignore and only. It is hard to say however howmany people that might be.
  2. This PR makes new OptionManager().init() and the transform methods return null if the file was ignored right now, instead of the merged config object for OptionManager, and {ignored: true} for transform like in Babel 6. We could consider keeping the old transform result value if we wanted though.
  3. This removes the util property from babel-core, which had exposed .canCompile and .shouldIgnore, since callers should just call .transform or use OptionManager (though we should expose a better interface for that) and check the return value to see if it was ignored.
  4. In babel-cli, --copy-files will now copy files that were ignored, where before it copied all non-compiled files, and skipped ignored files. Should be more what users expected anyway.
  5. ignore and only have to be an array when passed to Babel, so the CLIs handle the conversion internally instead of relying on core to do it.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 17, 2017

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

mention-bot commented Mar 17, 2017

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

builder.findConfigs(filename);
}
} catch (e) {
if (e.code !== "BABEL_IGNORED_FILE") throw e;

This comment has been minimized.

@loganfsmyth

loganfsmyth Mar 17, 2017

Member

I don't love this so if people want me to revisit I can, it's just the bailing out of a deep merge is kind of a pain with the current setup unfortunately.

@loganfsmyth

loganfsmyth Mar 17, 2017

Member

I don't love this so if people want me to revisit I can, it's just the bailing out of a deep merge is kind of a pain with the current setup unfortunately.

This comment has been minimized.

@hzoo

hzoo Mar 17, 2017

Member

lol, can leave a todo I suppose.. kinda gross

@hzoo

hzoo Mar 17, 2017

Member

lol, can leave a todo I suppose.. kinda gross

@hzoo hzoo added this to the Babel 7 milestone Mar 17, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Mar 17, 2017

Codecov Report

Merging #5487 into 7.0 will decrease coverage by 0.16%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##              7.0    #5487      +/-   ##
==========================================
- Coverage   85.48%   85.32%   -0.17%     
==========================================
  Files         201      201              
  Lines        9512     9483      -29     
  Branches     2696     2688       -8     
==========================================
- Hits         8131     8091      -40     
- Misses        893      899       +6     
- Partials      488      493       +5
Impacted Files Coverage Δ
packages/babel-core/src/util.js 83.33% <ø> (-11.59%)
packages/babel-cli/src/babel/file.js 76.92% <0%> (+0.97%)
...ckages/babel-core/src/transformation/file/index.js 83.84% <100%> (-0.31%)
packages/babel-core/src/index.js 100% <100%> (ø)
.../src/transformation/file/options/option-manager.js 82.11% <100%> (+1.01%)
packages/babel-core/src/transformation/pipeline.js 66.66% <50%> (-1.42%)
packages/babel-cli/src/babel-external-helpers.js 77.77% <60%> (-22.23%)
packages/babel-cli/src/babel/dir.js 56.09% <60%> (+2.43%)
packages/babel-cli/src/babel/index.js 67.94% <64.7%> (-0.89%)
.../transformation/file/options/build-config-chain.js 85.27% <71.79%> (-8.48%)
... and 7 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 b6194a8...b28f3b9. Read the comment docs.

codecov bot commented Mar 17, 2017

Codecov Report

Merging #5487 into 7.0 will decrease coverage by 0.16%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##              7.0    #5487      +/-   ##
==========================================
- Coverage   85.48%   85.32%   -0.17%     
==========================================
  Files         201      201              
  Lines        9512     9483      -29     
  Branches     2696     2688       -8     
==========================================
- Hits         8131     8091      -40     
- Misses        893      899       +6     
- Partials      488      493       +5
Impacted Files Coverage Δ
packages/babel-core/src/util.js 83.33% <ø> (-11.59%)
packages/babel-cli/src/babel/file.js 76.92% <0%> (+0.97%)
...ckages/babel-core/src/transformation/file/index.js 83.84% <100%> (-0.31%)
packages/babel-core/src/index.js 100% <100%> (ø)
.../src/transformation/file/options/option-manager.js 82.11% <100%> (+1.01%)
packages/babel-core/src/transformation/pipeline.js 66.66% <50%> (-1.42%)
packages/babel-cli/src/babel-external-helpers.js 77.77% <60%> (-22.23%)
packages/babel-cli/src/babel/dir.js 56.09% <60%> (+2.43%)
packages/babel-cli/src/babel/index.js 67.94% <64.7%> (-0.89%)
.../transformation/file/options/build-config-chain.js 85.27% <71.79%> (-8.48%)
... and 7 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 b6194a8...b28f3b9. Read the comment docs.

* Test if a filename ends with a compilable extension.
*/
export function canCompile(filename: string, altExts?: Array<string>): boolean {

This comment has been minimized.

@loganfsmyth

loganfsmyth Mar 17, 2017

Member

Curious for people's thoughts on this one. The current name is misleading since it's not really about what Babel can compile so much as providing the standard file extensions for Babel. Even then, it seems to me like it should be up to the users of babel-core to decide what to run through it.

That said, I do somewhat see the benefit of having a standardized set of extensions. What we could do is expose a babel.standardExtensions, but even then I'm not clear what we get from that vs tools deciding on their own. It's also something I'd be hesitant to change without a major version bump anyway, since who knows what people will be using in the wild.

@loganfsmyth

loganfsmyth Mar 17, 2017

Member

Curious for people's thoughts on this one. The current name is misleading since it's not really about what Babel can compile so much as providing the standard file extensions for Babel. Even then, it seems to me like it should be up to the users of babel-core to decide what to run through it.

That said, I do somewhat see the benefit of having a standardized set of extensions. What we could do is expose a babel.standardExtensions, but even then I'm not clear what we get from that vs tools deciding on their own. It's also something I'd be hesitant to change without a major version bump anyway, since who knows what people will be using in the wild.

This comment has been minimized.

@loganfsmyth

loganfsmyth Mar 17, 2017

Member

I ended up backing down on removing the extensions, just canCompile. Expose babel.DEFAULT_EXTENSIONS so we can still drop util.

@loganfsmyth

loganfsmyth Mar 17, 2017

Member

I ended up backing down on removing the extensions, just canCompile. Expose babel.DEFAULT_EXTENSIONS so we can still drop util.

if (opts.cache === false) cache = null;
delete opts.extensions;
delete opts.ignore;
delete opts.cache;

This comment has been minimized.

@hzoo

hzoo Mar 17, 2017

Member

never seen this one? would be nice to not nice delete everywhere. Oh and also not modify opts I suppose.. if we care about doing #3781

@hzoo

hzoo Mar 17, 2017

Member

never seen this one? would be nice to not nice delete everywhere. Oh and also not modify opts I suppose.. if we care about doing #3781

This comment has been minimized.

@loganfsmyth

loganfsmyth Mar 17, 2017

Member

These deletes are removing options that are specific to babel-register, so this is deleting it because babel-core will throw on the unknown option.

@loganfsmyth

loganfsmyth Mar 17, 2017

Member

These deletes are removing options that are specific to babel-register, so this is deleting it because babel-core will throw on the unknown option.

This comment has been minimized.

@sudo-suhas

sudo-suhas Jun 25, 2017

Sorry for commenting on an old thread but is there any reason you are not using lodash.omit? Another instance

@sudo-suhas

sudo-suhas Jun 25, 2017

Sorry for commenting on an old thread but is there any reason you are not using lodash.omit? Another instance

@@ -2,6 +2,10 @@ import * as t from "../lib";
import { assert } from "chai";
describe("converters", function () {
it("toIdentifier", function () {
assert.equal(t.toIdentifier("swag-lord"), "swagLord");

This comment has been minimized.

@hzoo

hzoo Mar 17, 2017

Member

lol gotta keep this one huh 🙂

@hzoo

hzoo Mar 17, 2017

Member

lol gotta keep this one huh 🙂

@hzoo

hzoo approved these changes Mar 17, 2017

just a few comments!

@@ -13,13 +12,24 @@ import pkg from "../package.json";
const program = new commander.Command("babel-node");
function collect(value, previousValue): Array<string> {

This comment has been minimized.

@existentialism

existentialism Mar 17, 2017

Member

Not a big deal, but worth creating cli-utils or something for this and booleanify?

@existentialism

existentialism Mar 17, 2017

Member

Not a big deal, but worth creating cli-utils or something for this and booleanify?

This comment has been minimized.

@loganfsmyth

loganfsmyth Mar 17, 2017

Member

I thought about it, but it's honestly such a small thing at I didn't care.

@loganfsmyth

loganfsmyth Mar 17, 2017

Member

I thought about it, but it's honestly such a small thing at I didn't care.

@loganfsmyth loganfsmyth merged commit 39c862c into babel:7.0 Mar 17, 2017

1 of 3 checks passed

codecov/patch 70% of diff hit (target 85.48%)
Details
codecov/project 85.32% (-0.17%) compared to b6194a8
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@loganfsmyth loganfsmyth deleted the loganfsmyth:relative-ignore-only branch Mar 17, 2017

pvdlg added a commit to semantic-release/cli that referenced this pull request Nov 24, 2017

pvdlg added a commit to semantic-release/cli that referenced this pull request Nov 24, 2017

@good-idea good-idea referenced this pull request Apr 3, 2018

Open

[WIP] Support Babel 7 #83

2 of 5 tasks complete

@BeauBouchard BeauBouchard referenced this pull request Aug 21, 2018

Open

babel 7 #346

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment