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 fixes + Move babel-core config processing from transformation/file/options into top-level folder #5489

Merged
merged 16 commits into from Mar 21, 2017

Conversation

@loganfsmyth
Copy link
Member

loganfsmyth commented Mar 17, 2017

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

As discussed on Slack, this moves files around in babel-core so that all of the config loading is 100% independent from the transformation process.

Specifically converting:

packages/babel-core/src/
├── helpers
│   ├── environment.js
│   ├── get-possible-plugin-names.js
│   ├── get-possible-preset-names.js
│   ├── merge.js
│   ├── normalize-ast.js
│   ├── resolve-from-possible-names.js
│   ├── resolve-plugin.js
│   ├── resolve-preset.js
│   └── resolve.js
├── index.js
├── store.js
├── tools
│   └── build-external-helpers.js
├── transformation
│   ├── file
│   │   ├── index.js
│   │   ├── metadata.js
│   │   └── options
│   │       ├── build-config-chain.js
│   │       ├── option-manager.js
│   │       └── removed.js
│   ├── internal-plugins
│   │   ├── block-hoist.js
│   │   └── shadow-functions.js
│   ├── pipeline.js
│   ├── plugin-pass.js
│   └── plugin.js
└── util.js

to

packages/babel-core/src/
├── config
│   ├── build-config-chain.js
│   ├── helpers
│   │   ├── environment.js
│   │   ├── get-possible-plugin-names.js
│   │   ├── get-possible-preset-names.js
│   │   ├── merge.js
│   │   ├── resolve-from-possible-names.js
│   │   ├── resolve-plugin.js
│   │   ├── resolve-preset.js
│   │   └── resolve.js
│   ├── index.js
│   ├── option-manager.js
│   ├── plugin.js
│   └── removed.js
├── index.js
├── tools
│   └── build-external-helpers.js
└── transformation
    ├── file
    │   ├── index.js
    │   └── metadata.js
    ├── internal-plugins
    │   ├── block-hoist.js
    │   └── shadow-functions.js
    ├── pipeline.js
    ├── plugin-pass.js
    └── store.js

More work to come after this to simplify config but this is the best start to split things.

Also flagged this as a breaking change since it included some misc refactoring. Breaking:

  1. More strict parsing to help with error feedback

  2. Presets that returned string values resolved them inconsistently, e.g.

    {
      presets: ['foo'],
    }
    

    would load babel-preset-foo, and if that preset had {plugins: ['string-plugin']} it would resolve string-plugin relative to the preset location.

    But if someone loaded that preset from

    {
      presets: [require('babel-preset-foo')],
    }
    

    then Babel doesn't know where the preset came from. In this case, it would resolve string presets relative to the current working directory.

    Now Babel 7 will consistently resolve the string relative to the configuration file that referenced the preset, or working directory for programmatic args, since that's the only location we know consistently.

  3. Plugin instance no longer available at state.plugin, which noone probably used anyway?

  4. Parser and generator strings are solved at config processing time, so they will be functions in manipulateOptions callback

Personally I see none of these as high-enough impact to even be worth documenting specifically.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Mar 17, 2017

Hey @loganfsmyth! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

cd packages/babel-runtime; \
	node scripts/build-dist.js
module.js:472
    throw err;
    ^
Error: Cannot find module '../../babel-core/lib/util'
options: preset,
alias: presetLoc,
loc: presetLoc,
dirname: dirname,

This comment has been minimized.

Copy link
@hzoo

hzoo Mar 17, 2017

Member

can do shorthand 😄

import traverse from "babel-traverse";
import clone from "lodash/clone";

const GLOBAL_VISITOR_PROPS = ["enter", "exit"];

export default class Plugin extends Store {

This comment has been minimized.

Copy link
@hzoo

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Mar 17, 2017

Author Member

this in that example is an instance of PluginPass, not Plugin. The only place Plugin was ever even exposed was this.plugin which I'm removing in this PR too, so removing the subclassing won't hurt anything that isn't already broken by that.

This comment has been minimized.

Copy link
@hzoo

hzoo Mar 17, 2017

Member

Ah ok I couldn't tell so I just assumed it was Plugin (and of course the tests passes so it's good, just was confused)

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Mar 17, 2017

Author Member

Yeah understanding where each type of class is used is super confusing.

import sortBy from "lodash/sortBy";

export default new Plugin({

This comment has been minimized.

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Mar 17, 2017

Author Member

They still need to be Plugin instances eventually, but now I'm just relying on the standard plugin initialization interface via https://github.com/babel/babel/pull/5489/files#diff-d6903991e1c21aaacc4d615157f587e5R30 instead of manually calling new Plugin.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 17, 2017

Codecov Report

Merging #5489 into 7.0 will decrease coverage by 0.08%.
The diff coverage is 64.63%.

@@            Coverage Diff             @@
##              7.0    #5489      +/-   ##
==========================================
- Coverage   85.39%   85.31%   -0.09%     
==========================================
  Files         201      200       -1     
  Lines        9483     9491       +8     
  Branches     2688     2696       +8     
==========================================
- Hits         8098     8097       -1     
- Misses        892      894       +2     
- Partials      493      500       +7
Impacted Files Coverage Δ
...es/babel-core/src/config/helpers/resolve-preset.js 100% <ø> (ø)
...kages/babel-core/src/config/helpers/environment.js 100% <ø> (ø)
...re/src/config/helpers/get-possible-preset-names.js 100% <ø> (ø)
...src/transformation/internal-plugins/block-hoist.js 100% <ø> (ø) ⬆️
...ckages/babel-core/src/config/build-config-chain.js 85.27% <ø> (ø)
packages/babel-core/src/config/helpers/resolve.js 100% <ø> (ø)
...ransformation/internal-plugins/shadow-functions.js 100% <ø> (ø) ⬆️
...re/src/config/helpers/get-possible-plugin-names.js 100% <ø> (ø)
packages/babel-core/src/config/plugin.js 75% <ø> (ø)
packages/babel-core/src/transformation/store.js 100% <ø> (ø)
... and 17 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 39c862c...7551a8e. Read the comment docs.

passes.push(pass);
visitors.push(plugin.visitor);

if (plugin.pre) plugin.pre.call(pass, this);

This comment has been minimized.

Copy link
@hzoo

hzoo Mar 17, 2017

Member

What's this call for? looks like you are doing this right below?

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Mar 17, 2017

Author Member

Woops, I noticed this and apparently forgot to remove it, good catch.

This comment has been minimized.

Copy link
@hzoo

hzoo Mar 17, 2017

Member

k I think just removing this and :shipit:


// With "passPerPreset" enabled there may still be presets in the options.
if (mergedOpts.presets) {
passes = passes.concat(mergedOpts.presets.map((preset) => preset.plugins).filter(Boolean));

This comment has been minimized.

Copy link
@hzoo

hzoo Mar 17, 2017

Member

I guess passes = [...passes, ...mergedOpts.presets.map((preset) => preset.plugins).filter(Boolean)]; isn't any more readable

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Mar 17, 2017

Author Member

Yaa, neither are pretty. This will probably be removed in my rewrite anyway though.

@hzoo hzoo added this to the Babel 7 milestone Mar 17, 2017
@hzoo
hzoo approved these changes Mar 17, 2017
Copy link
Member

hzoo left a comment

question was: #5489 (comment)

concern was: 38720ae#r106663816

seems like the transform-runtime plugin uses a method from Store?

hzoo added 2 commits Mar 21, 2017
@hzoo hzoo merged commit 7d37017 into babel:7.0 Mar 21, 2017
1 of 3 checks passed
1 of 3 checks passed
codecov/patch 64.63% of diff hit (target 85.39%)
Details
codecov/project 85.31% (-0.09%) compared to 39c862c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@loganfsmyth loganfsmyth deleted the loganfsmyth:conf-refactor branch Mar 21, 2017
@lock lock bot added the outdated 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.
Projects
None yet
3 participants
You can’t perform that action at this time.