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

Refactor OptionManager to be a short class with a bunch of pure helper functions. #5602

Merged
merged 11 commits into from Apr 14, 2017

Conversation

@loganfsmyth
Copy link
Member

loganfsmyth commented Apr 7, 2017

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

Last PR before I post the changes to OptionManager for caching. This PR is basically slowly moving things around and simplifying OptionManager, so it no longer has all kinds of hard to follow statics. It's a simple class 100 lines long, with a bunch of pure functions that it uses to slowly build up the config info.

Review-wise it might be easier to just look at the final option-manager.js file to say what you think?

@loganfsmyth loganfsmyth added this to the Babel 7 milestone Apr 7, 2017
@loganfsmyth loganfsmyth requested a review from hzoo Apr 7, 2017
@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Apr 7, 2017

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

Copy link
Member

xtuc left a comment

Looking good! Great work @loganfsmyth 👍


let inheritsDescriptor;
let inherits;
if (plugin.inherits) {

This comment has been minimized.

Copy link
@xtuc

xtuc Apr 7, 2017

Member

I had the discussion in a PR last time, could we support multiple inheritance?

{
  inherits: require('foo')
}

vs

{
  inherits: [
    require('foo'),
    require('bar'),
  ]
}

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Apr 7, 2017

Author Member

Yeah, definitely something we could add.

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Apr 7, 2017

Author Member

I think I'll leave it for a future PR, but it wouldn't be too bad to add if we want.

This comment has been minimized.

Copy link
@xtuc

xtuc Apr 7, 2017

Member

Yes sure. I will note this in my todo list.

}

OptionManager.memoisedPlugins = [];
function createBareOptions() {

This comment has been minimized.

Copy link
@xtuc

xtuc Apr 7, 2017

Member

This is more createInitialOptions?

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Apr 7, 2017

Author Member

Yeah, I just left the name we had in there already, but I can change it too.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 7, 2017

Codecov Report

Merging #5602 into 7.0 will increase coverage by 0.03%.
The diff coverage is 75.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##              7.0    #5602      +/-   ##
==========================================
+ Coverage   84.45%   84.48%   +0.03%     
==========================================
  Files         285      285              
  Lines        9648     9644       -4     
  Branches     2711     2700      -11     
==========================================
  Hits         8148     8148              
+ Misses        998      996       -2     
+ Partials      502      500       -2
Impacted Files Coverage Δ
packages/babel-messages/src/index.js 61.53% <ø> (ø) ⬆️
...bel-core/src/config/loading/files/index-browser.js 0% <0%> (ø) ⬆️
packages/babel-core/src/config/index.js 100% <100%> (ø) ⬆️
packages/babel-core/src/config/plugin.js 100% <100%> (+25%) ⬆️
...ges/babel-core/src/config/loading/files/plugins.js 47.82% <50%> (ø) ⬆️
packages/babel-core/src/config/option-manager.js 77.77% <77.5%> (+0.97%) ⬆️
packages/babel-traverse/src/path/context.js 86.2% <0%> (ø) ⬆️
packages/babel-traverse/src/visitors.js 86.66% <0%> (+0.95%) ⬆️

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 2ea3338...248c240. Read the comment docs.

@loganfsmyth loganfsmyth force-pushed the loganfsmyth:option-manager-cleanup branch from 8a1d05e to 118d861 Apr 7, 2017
@loganfsmyth loganfsmyth force-pushed the loganfsmyth:option-manager-cleanup branch from 118d861 to 248c240 Apr 10, 2017
@@ -146,7 +146,7 @@ describe("api", function () {
plugins: [__dirname + "/../../babel-plugin-syntax-jsx", false],
});
},
/TypeError: \[BABEL\] unknown: Falsy value found in plugins/

This comment has been minimized.

Copy link
@hzoo

hzoo Apr 14, 2017

Member

So we are ok just saying this is "falsy"?

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Apr 14, 2017

Author Member

It does append the value itself now, before it didn't.

This comment has been minimized.

Copy link
@hzoo

hzoo Apr 14, 2017

Member

nvm I was talking about it saying it was a plugin/preset before but maybe that's not a big deal

@hzoo
hzoo approved these changes Apr 14, 2017
Copy link
Member

hzoo left a comment

looks good, crazy stuff (I went through using --inspect)

@loganfsmyth loganfsmyth merged commit ad250ef into babel:7.0 Apr 14, 2017
3 checks passed
3 checks passed
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 84.48% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@loganfsmyth loganfsmyth deleted the loganfsmyth:option-manager-cleanup branch Apr 14, 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
4 participants
You can’t perform that action at this time.