-
-
Notifications
You must be signed in to change notification settings - Fork 158
separate default builtins for platforms #226
Conversation
src/index.js
Outdated
const isAnyTarget = !targetNames.length; | ||
const isWebTarget = targetNames.some((name) => name !== "node"); | ||
|
||
return (isAnyTarget || isWebTarget) ? defaultInclude : []; |
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.
should we rename it?
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.
Maybe defaultInclude
-> defaultWebIncludes
?
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.
yeah since import defaultInclude from "./default-includes";
Codecov Report
@@ Coverage Diff @@
## master #226 +/- ##
==========================================
+ Coverage 92.85% 93.06% +0.21%
==========================================
Files 3 4 +1
Lines 196 202 +6
Branches 59 60 +1
==========================================
+ Hits 182 188 +6
Misses 9 9
Partials 5 5
Continue to review full report at Codecov.
|
function getPlatformSpecificDefaultFor(targets) { | ||
const targetNames = Object.keys(targets); | ||
const isAnyTarget = !targetNames.length; | ||
const isWebTarget = targetNames.some((name) => name !== "node"); |
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.
maybe check for electron
and uglify
also?
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.
but you would want to include it for electron/uglify right?
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.
If we have node + electron or node + uglify, isWebTarget will be true. But we don't need to include it in these cases ?
fcc1e6b
to
3029b54
Compare
issue #220