-
-
Notifications
You must be signed in to change notification settings - Fork 158
Conversation
src/index.js
Outdated
@@ -143,16 +143,24 @@ export const getTargets = (targets = {}) => { | |||
return targetOps; | |||
}; | |||
|
|||
export const validateBoolOption = (name, value, defaultValue) => { | |||
if (typeof value === "undefined") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good place to use Lodash's isUndefined
and isBoolean
since it's already in the package.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this particular case it's part of the plugin so we would just need to move the dep to deps instead of devDeps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, fair point. Is that something we want to avoid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end it's a build tool so we should use when we would like to - hard for us just to implement everything when it's not needed but yeah something I've been struggling with sometimes. Sure we want to keep low dependencies and everyone complaining about Babel install times but hey we aren't getting any help and it just makes developing that much harder
Nice! But I want to mention other options too. If I'm not mistaken, For example, I want to run transform-es2015-arrow-functions with Diving deeper into the code, we may found that many plugins receive options they don't know and don't need (most). Just topic to discuss rather after PR. During this stage, this PR is a good progress. |
@yavorsky that was an issue with preset options when we originally talked about them - users wanted complete control and sebastian didn't want options at all. Logan suggested we allow options with predetermined options at first which is what we have atm It's true a user might want true for 1 thing and false without but dono how much configurability we want |
We should add a spec "integration" test in fixtures. I can do this and then merge |
@hzoo rebased and added test |
Codecov Report
@@ Coverage Diff @@
## master #98 +/- ##
=========================================
+ Coverage 93.03% 93.2% +0.16%
=========================================
Files 4 4
Lines 201 206 +5
Branches 59 60 +1
=========================================
+ Hits 187 192 +5
Misses 9 9
Partials 5 5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fine, dono if we want it in 1.x or 2.x
e5e53da
to
367bace
Compare
Passes through
spec
to plugins.I think this only affects arrow functions and template strings, but I haven't verified.