Replace underscore by lodash #6030

Kureev · 2016-02-19T11:16:01Z

As far as we agreed to merge rnpm into react-native core, we need to align our dependencies to exclude duplications. One of the steps forward would be to use the same utilities library. According to the thread on fb, everybody is fine with replacing underscore by lodash (which we use internally for rnpm).

So, here we go!

cc @mkonicek @davidaurelio @grabbou

Test plan

$ npm test

Code formatting

Changes are aligned with the current style guide.

facebook-github-bot · 2016-02-19T11:16:22Z

By analyzing the blame information on this pull request, we identified @vjeux, @ide and @martinbigio to be potential reviewers.

grabbou · 2016-02-19T11:20:19Z

Thanks, looks good to me. I am pretty sure you will have to squash commits, but you can probably wait till someone confirms nothing to be changed here.

Kureev · 2016-02-19T11:22:34Z

@grabbou Yeah, I'll keep them separated for now (easier to revert if needed), but later on I can squash them into one.

satya164 · 2016-02-19T11:38:41Z

@grabbou

I am pretty sure you will have to squash commits

The bot will squash commits when this gets merged :)

bestander · 2016-02-19T12:03:11Z

@satya164, when it is ready, instead of shipit do import, and ping me.
I'll need to update npm deps internally

satya164 · 2016-02-19T12:04:11Z

@bestander Okies. I will :)

satya164 · 2016-02-19T12:16:03Z

Can we require individual modules instead of the whole package? For example require('lodash/flattenDeep')?

Doesn't make any difference since it's just the packager though.

gaearon · 2016-02-19T12:18:10Z

Can we require individual modules instead of the whole package? For example require('lodash/flattenDeep')?

Agree that this would be nicer. Less code to process is better 👍

satya164 · 2016-02-19T12:26:35Z

packager/react-packager/index.js

  return createServer(options);
 }

+function omit(obj, blacklistedKeys) {


Why not use lodash/omit ?

Ideally, I'd like to replace lodash by a self-written functions (to eliminate one of the ext. dependencies). But I think we shouldn't include it in this PR.

I think i'd be nice to just add a small test suite for that then.

@grabbou that would mean that we need to extract this function somewhere. I'd like to do it, but I have no clue where to put it. Any suggestions?

satya164 · 2016-02-19T12:31:35Z

Looks good overall. This is awesome, because lots of other packages are using lodash anyways. The less deps, the better.

Kureev · 2016-02-19T12:46:48Z

I agree about individual modules, but I remember in the facebook thread @davidaurelio had some objections about it.

satya164 · 2016-02-19T13:05:23Z

@Kureev

I agree about individual modules, but I remember in the facebook thread @davidaurelio had some objections about it.

Let's wait for @davidaurelio 's feedback then.

@satya164 Ideally, I'd like to replace lodash by self-written functions (to eliminate one of the ext. dependencies).

IMO it's not worth it, lodash is used by lots of packages. So we're not really getting rid of it.

Kureev · 2016-02-19T13:13:31Z

@satya164 maybe you're right. But as you said, let's wait for the @davidaurelio's feedback first 👍

mkonicek · 2016-02-19T15:39:08Z

Thank you for doing this!

satya164 · 2016-02-24T10:36:24Z

ping @davidaurelio

mkonicek · 2016-02-25T23:57:13Z

David is very busy trying to merge the PRs that will unbreak Travis. Let's please hold on with this until Travis is green. Thanks for the patience!

satya164 · 2016-03-04T20:18:26Z

ping @davidaurelio

…by native map implementation

facebook-github-bot · 2016-03-04T21:52:20Z

@Kureev updated the pull request.

Kureev · 2016-03-04T21:52:56Z

I rebased this branch by the latest master. Now tests should be all green 🍏

davidaurelio · 2016-03-05T14:49:21Z

@facebook-github-bot shipit

davidaurelio · 2016-03-05T14:49:38Z

I will hold this until Monday. It will get in the next version.

facebook-github-bot · 2016-03-05T14:50:33Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

Kureev · 2016-03-05T15:15:43Z

👍

mkonicek · 2016-03-07T21:54:16Z

Hey guys, I'm merging #6346 and then this one right after.

davidaurelio · 2016-03-07T21:59:17Z

thanks @mkonicek

mkonicek · 2016-03-07T23:50:08Z

OK #6346 is landing internally now, once it lands want to rebase this one on top of it because both require updating the npm-shrinkwrap.json file. It's late in London now so will continue in the morning. Thanks for the patience!

Kureev · 2016-03-08T09:18:31Z

ping me when I can rebase

mkonicek · 2016-03-08T14:52:40Z

Turns out #6346 doesn't work with our internal build based on Buck so we can probably merge this one first.

mkonicek · 2016-03-08T14:53:02Z

(oops wrong button 😃 )

mkonicek · 2016-03-08T15:32:12Z

OK I did some investigation and this PR is in fact blocked on #6346. When I try merging this PR on its own and regenerate npm-shrinkwrap.json the packager doesn't work when used with npm 2.

The root of the problem is we can't regenerate npm-shrinkwrap.json in a way that works with npm 2, and since changes to package.json require npm-shrinkwrap.json to be regenerated, we can't merge any PRs that change package.json until #6346 lands.

Kureev · 2016-03-08T16:11:32Z

davidaurelio · 2016-03-08T23:44:12Z

Can we get away with lodash 3? babel depends on that. In that case, npm install puts three versions of lodash in the tree. If we need v4, we end up with 20.

davidaurelio · 2016-03-08T23:44:38Z

no need to update the PR. I have it updated internally

Kureev · 2016-03-09T08:49:09Z

@davidaurelio sure, thanks! I think it should work even without any code changes (except the lodash version in package.json).

@grabbou we probably need to follow up on this in the link and install plugins (just to be sure that it works with lodash@3 without any code changes).

grabbou · 2016-03-09T08:59:47Z

Yeah, let's apply updates to our internal packages once this is merged.

Summary:As far as we agreed to merge `rnpm` into react-native core, we need to align our dependencies to exclude duplications. One of the steps forward would be to use the same utilities library. According to the thread on fb, everybody is fine with replacing underscore by lodash (which we use internally for rnpm). So, here we go! cc mkonicek davidaurelio grabbou **Test plan** ``` $ npm test ``` ![image](https://cloud.githubusercontent.com/assets/2273613/13173972/ee34c922-d700-11e5-971b-68ff7322b6d6.png) **Code formatting** Changes are aligned with the current [style guide](https://github.com/facebook/react-native/blob/master/CONTRIBUTING.md#style-guide). Closes facebook#6030 Differential Revision: D3016271 Pulled By: davidaurelio fb-gh-sync-id: c4f6776a7de7470283d3ca5a8b56e423247f5e45 shipit-source-id: c4f6776a7de7470283d3ca5a8b56e423247f5e45

Summary:As far as we agreed to merge `rnpm` into react-native core, we need to align our dependencies to exclude duplications. One of the steps forward would be to use the same utilities library. According to the thread on fb, everybody is fine with replacing underscore by lodash (which we use internally for rnpm). So, here we go! cc mkonicek davidaurelio grabbou **Test plan** ``` $ npm test ``` ![image](https://cloud.githubusercontent.com/assets/2273613/13173972/ee34c922-d700-11e5-971b-68ff7322b6d6.png) **Code formatting** Changes are aligned with the current [style guide](https://github.com/facebook/react-native/blob/master/CONTRIBUTING.md#style-guide). Closes facebook/react-native#6030 Differential Revision: D3016271 Pulled By: davidaurelio fb-gh-sync-id: c4f6776a7de7470283d3ca5a8b56e423247f5e45 shipit-source-id: c4f6776a7de7470283d3ca5a8b56e423247f5e45

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Feb 19, 2016

satya164 added GH Review: needs-revision and removed GH Review: review-needed labels Feb 19, 2016

satya164 reviewed Feb 19, 2016
View reviewed changes

Kureev mentioned this pull request Feb 21, 2016

Replace atomic lodash packages by a single one rnpm/rnpm#95

Merged

Kureev Alexey added 8 commits March 4, 2016 21:34

Remove underscore from HMRBundle

42a9183

Replace _.pluck by the native map implementation in BundleBase.js

82ff57b

Replace underscore by lodash in the formatBanner.js

116cb26

Remove underscore from SocketInterface and it's test

abd0a6b

Replace underscore's omit by the native implementation

ef70910

Replaced underscore by lodash in Bundler/Bundle.js; Replaced _.pluck …

f8b779e

…by native map implementation

Chnage mocking rule for Bundler/Bundle-test

5fd8fd2

Replace underscore by lodash in packager/src/Server folder

5edf511

Kureev force-pushed the chore/replace-underscore-by-lodash branch from a165520 to d9695ff Compare March 4, 2016 21:51

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Mar 4, 2016

facebook-github-bot added GH Review: accepted and removed GH Review: review-needed labels Mar 5, 2016

mkonicek closed this Mar 8, 2016

mkonicek reopened this Mar 8, 2016

Kureev mentioned this pull request Mar 9, 2016

Downgrade lodash rnpm/rnpm#105

Merged

ghost closed this in db3a00d Mar 9, 2016

This pull request was closed.

Replace underscore by lodash #6030

Replace underscore by lodash #6030

Uh oh!

Conversation

Kureev commented Feb 19, 2016

Uh oh!

facebook-github-bot commented Feb 19, 2016

Uh oh!

grabbou commented Feb 19, 2016

Uh oh!

Kureev commented Feb 19, 2016

Uh oh!

satya164 commented Feb 19, 2016

Uh oh!

bestander commented Feb 19, 2016

Uh oh!

satya164 commented Feb 19, 2016

Uh oh!

satya164 commented Feb 19, 2016

Uh oh!

gaearon commented Feb 19, 2016

Uh oh!

satya164 Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

Kureev Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

grabbou Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

Kureev Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

satya164 commented Feb 19, 2016

Uh oh!

Kureev commented Feb 19, 2016

Uh oh!

satya164 commented Feb 19, 2016

Uh oh!

Kureev commented Feb 19, 2016

Uh oh!

mkonicek commented Feb 19, 2016

Uh oh!

satya164 commented Feb 24, 2016

Uh oh!

mkonicek commented Feb 25, 2016

Uh oh!

satya164 commented Mar 4, 2016

Uh oh!

facebook-github-bot commented Mar 4, 2016

Uh oh!

Kureev commented Mar 4, 2016

Uh oh!

davidaurelio commented Mar 5, 2016

Uh oh!

davidaurelio commented Mar 5, 2016

Uh oh!

facebook-github-bot commented Mar 5, 2016

Uh oh!

Kureev commented Mar 5, 2016

Uh oh!

mkonicek commented Mar 7, 2016

Uh oh!

davidaurelio commented Mar 7, 2016

Uh oh!

mkonicek commented Mar 7, 2016

Uh oh!

Kureev commented Mar 8, 2016

Uh oh!

mkonicek commented Mar 8, 2016

Uh oh!

mkonicek commented Mar 8, 2016

Uh oh!

mkonicek commented Mar 8, 2016

Uh oh!

Kureev commented Mar 8, 2016

Uh oh!

davidaurelio commented Mar 8, 2016

Uh oh!