Skip to content
This repository has been archived by the owner on May 11, 2018. It is now read-only.

separate default builtins for platforms #226

Merged
merged 2 commits into from Mar 27, 2017

Conversation

mshustov
Copy link

issue #220

src/index.js Outdated
const isAnyTarget = !targetNames.length;
const isWebTarget = targetNames.some((name) => name !== "node");

return (isAnyTarget || isWebTarget) ? defaultInclude : [];
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we rename it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe defaultInclude -> defaultWebIncludes?

Copy link
Member

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-io
Copy link

codecov-io commented Mar 24, 2017

Codecov Report

Merging #226 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/normalize-options.js 88.57% <ø> (ø) ⬆️
src/index.js 95.76% <100%> (+0.18%) ⬆️
src/default-includes.js 100% <100%> (ø)

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 a4d585c...3029b54. Read the comment docs.

function getPlatformSpecificDefaultFor(targets) {
const targetNames = Object.keys(targets);
const isAnyTarget = !targetNames.length;
const isWebTarget = targetNames.some((name) => name !== "node");
Copy link
Member

@yavorsky yavorsky Mar 25, 2017

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?

Copy link
Member

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?

Copy link
Member

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 ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants