chore: add lodash/fp #19

AfonsoVReis · 2018-02-19T18:51:43Z

Closes #18 .

codecov · 2018-02-19T18:56:04Z

Codecov Report

Merging #19 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #19   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          20     21    +1     
  Branches        6      6           
=====================================
+ Hits           20     21    +1

Impacted Files	Coverage Δ
index.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 5e4ede1...2c30efe. Read the comment docs.

andregoncalvesdev

LGTM.

I would just change the commit message to something more explicit like:

chore: add lodash/fp as default id on babel-plugin-lodash

andregoncalvesdev · 2018-02-19T19:53:45Z

This is just an opinion but defaultLodashOptionsIds sounds overridable. I prefer baseLodasOptionsIds as it sounds more extensible like this case.

satazor · 2018-02-19T20:35:46Z

index.js

+            [require.resolve('babel-plugin-lodash')],
+            Object.assign({},
+                options.lodash,
+                { id: options.lodash.id ? defaultLodashOptionsIds.concat(options.lodash.id) : defaultLodashOptionsIds }),


We should document the defaults in the README and perhaps add an example adding recompose?

I agree, i will add the information with an example in the README.

satazor · 2018-02-19T20:36:21Z

I agree with having a more specific commit message.

satazor · 2018-02-20T03:28:10Z

index.js

@@ -94,9 +94,13 @@ module.exports = (context, options) => {

    // Cherry-pick lodash modules for smaller bundles
    if (options.lodash) {
+        const defaultLodashOptionsIds = ['lodash/fp'];


Looking at https://github.com/lodash/babel-plugin-lodash/blob/master/src/config.js#L17, I think that when we pass id, it completely overrides the default ones. This means that we must include all those defaults.

It has been reported in this issue. lodash/babel-plugin-lodash#184 .
We can re-include the defaults, but we should be concerned about future fixes on this matter.

Closes #19 .

satazor · 2018-02-20T11:37:39Z

index.js

+            [require.resolve('babel-plugin-lodash')],
+            Object.assign({},
+                options.lodash,
+                { id: options.lodash.id ? baseLodashOptionsIds.concat(options.lodash.id) : baseLodashOptionsIds }),


Can this be changed to:

{ id: baseLodashOptionsIds.concat(options.lodash.id || []) }

? It's a bit simpler.

satazor · 2018-02-20T11:38:31Z

test/__snapshots__/index.spec.js.snap

+          "lodash-es",
+          "lodash-compat",
+          "lodash/fp",
+          "lodash-compat",


There's a repeated lodash-compat here. Can you change the test to use recompose instead?

satazor · 2018-02-20T11:43:37Z

README.md

+
+Transform to cherry-pick Lodash modules is enabled. 
+
+`lodash-es` `lodash-compat` and `lodash/fp` are added by default, but you can cherry-pick any module you need as long as you specify it on `options.id`.


Could we change this part to:

Specify which modules will have the cherry-pick transformation applied.

Note that lodash-es, lodash-compat and lodash/fp are always added for you, regardless of having this option defined or not.

For instance, to have smaller bundles when using recompose:

satazor · 2018-02-20T12:28:12Z

README.md

+
+Note that `lodash-es`, `lodash-compat` and `lodash/fp` are always added for you, regardless of having this option defined or not.
+
+For instance, to have smaller bundles when using [recompose](https://github.com/acdlite/recompose) :


Please remove extra space before :.

satazor · 2018-02-20T14:56:13Z

@AfonsoVReis I would consider this a feat don't you think @andregoncalvesdev ?

andregoncalvesdev · 2018-02-20T14:59:21Z

Yes @satazor , i agree this is feat.

Closes #18.

AfonsoVReis · 2018-02-20T15:02:11Z

Changed to feat instead of chore.

andregoncalvesdev reviewed Feb 19, 2018

View reviewed changes

satazor reviewed Feb 19, 2018

View reviewed changes

satazor requested changes Feb 20, 2018

View reviewed changes

AfonsoVReis force-pushed the add-lodashfp branch from 6441c21 to 922addf Compare February 20, 2018 10:37

AfonsoVReis added a commit that referenced this pull request Feb 20, 2018

chore: add lodash/fp as default id on babel-plugin-lodash

922addf

Closes #19 .

AfonsoVReis force-pushed the add-lodashfp branch from 922addf to 5f3a17a Compare February 20, 2018 10:38

satazor requested changes Feb 20, 2018

View reviewed changes

AfonsoVReis force-pushed the add-lodashfp branch from 5f3a17a to 9f9878e Compare February 20, 2018 12:06

satazor reviewed Feb 20, 2018

View reviewed changes

AfonsoVReis force-pushed the add-lodashfp branch from 9f9878e to e7d65c2 Compare February 20, 2018 13:36

satazor approved these changes Feb 20, 2018

View reviewed changes

feat: add lodash/fp as default id on babel-plugin-lodash

2c30efe

Closes #18.

AfonsoVReis force-pushed the add-lodashfp branch from e7d65c2 to 2c30efe Compare February 20, 2018 15:01

satazor merged commit 9694af3 into master Feb 20, 2018

satazor deleted the add-lodashfp branch February 20, 2018 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add lodash/fp #19

chore: add lodash/fp #19

AfonsoVReis commented Feb 19, 2018

codecov bot commented Feb 19, 2018 •

edited

Loading

andregoncalvesdev left a comment

andregoncalvesdev commented Feb 19, 2018

satazor Feb 19, 2018 •

edited

Loading

AfonsoVReis Feb 19, 2018

andregoncalvesdev Feb 19, 2018

satazor commented Feb 19, 2018

satazor Feb 20, 2018

AfonsoVReis Feb 20, 2018

satazor Feb 20, 2018 •

edited

Loading

andregoncalvesdev Feb 20, 2018

satazor Feb 20, 2018

satazor Feb 20, 2018 •

edited

Loading

satazor Feb 20, 2018

satazor commented Feb 20, 2018

andregoncalvesdev commented Feb 20, 2018

AfonsoVReis commented Feb 20, 2018


		Transform to cherry-pick Lodash modules is enabled.

		`lodash-es` `lodash-compat` and `lodash/fp` are added by default, but you can cherry-pick any module you need as long as you specify it on `options.id`.


		Note that `lodash-es`, `lodash-compat` and `lodash/fp` are always added for you, regardless of having this option defined or not.

		For instance, to have smaller bundles when using [recompose](https://github.com/acdlite/recompose) :

chore: add lodash/fp #19

chore: add lodash/fp #19

Conversation

AfonsoVReis commented Feb 19, 2018

codecov bot commented Feb 19, 2018 • edited Loading

Codecov Report

andregoncalvesdev left a comment

Choose a reason for hiding this comment

andregoncalvesdev commented Feb 19, 2018

satazor Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

AfonsoVReis Feb 19, 2018

Choose a reason for hiding this comment

andregoncalvesdev Feb 19, 2018

Choose a reason for hiding this comment

satazor commented Feb 19, 2018

satazor Feb 20, 2018

Choose a reason for hiding this comment

AfonsoVReis Feb 20, 2018

Choose a reason for hiding this comment

satazor Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

andregoncalvesdev Feb 20, 2018

Choose a reason for hiding this comment

satazor Feb 20, 2018

Choose a reason for hiding this comment

satazor Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

satazor Feb 20, 2018

Choose a reason for hiding this comment

satazor commented Feb 20, 2018

andregoncalvesdev commented Feb 20, 2018

AfonsoVReis commented Feb 20, 2018

codecov bot commented Feb 19, 2018 •

edited

Loading

satazor Feb 19, 2018 •

edited

Loading

satazor Feb 20, 2018 •

edited

Loading

satazor Feb 20, 2018 •

edited

Loading