Uh oh!
There was an error while loading. Please reload this page.
Replace underscore by lodash - #6030
Conversation
facebook-github-bot
commented
Feb 19, 2016
By analyzing the blame information on this pull request, we identified @vjeux, @ide and @martinbigio to be potential reviewers. |
grabbou
commented
Feb 19, 2016
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
commented
Feb 19, 2016
@grabbou Yeah, I'll keep them separated for now (easier to revert if needed), but later on I can squash them into one. |
satya164
commented
Feb 19, 2016
The bot will squash commits when this gets merged :) |
bestander
commented
Feb 19, 2016
@satya164, when it is ready, instead of |
satya164
commented
Feb 19, 2016
@bestander Okies. I will :) |
satya164
commented
Feb 19, 2016
Can we require individual modules instead of the whole package? For example Doesn't make any difference since it's just the packager though. |
gaearon
commented
Feb 19, 2016
Agree that this would be nicer. Less code to process is better 👍 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think i'd be nice to just add a small test suite for that then.
There was a problem hiding this comment.
@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
commented
Feb 19, 2016
Looks good overall. This is awesome, because lots of other packages are using |
Kureev
commented
Feb 19, 2016
I agree about individual modules, but I remember in the facebook thread @davidaurelio had some objections about it. |
satya164
commented
Feb 19, 2016
Let's wait for @davidaurelio 's feedback then.
IMO it's not worth it, |
Kureev
commented
Feb 19, 2016
@satya164 maybe you're right. But as you said, let's wait for the @davidaurelio's feedback first 👍 |
mkonicek
commented
Feb 19, 2016
Thank you for doing this! |
satya164
commented
Feb 24, 2016
ping @davidaurelio |
mkonicek
commented
Feb 25, 2016
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
commented
Mar 4, 2016
ping @davidaurelio |
…by native map implementation
a165520 to
d9695ffComparefacebook-github-bot
commented
Mar 4, 2016
@Kureev updated the pull request. |
Kureev
commented
Mar 4, 2016
I rebased this branch by the latest master. Now tests should be all green 🍏 |
davidaurelio
commented
Mar 5, 2016
@facebook-github-bot shipit |
davidaurelio
commented
Mar 5, 2016
I will hold this until Monday. It will get in the next version. |
facebook-github-bot
commented
Mar 5, 2016
Thanks for importing. If you are an FB employee go to Phabricator to review. |
Kureev
commented
Mar 5, 2016
👍 |
mkonicek
commented
Mar 7, 2016
Hey guys, I'm merging #6346 and then this one right after. |
davidaurelio
commented
Mar 7, 2016
thanks @mkonicek |
mkonicek
commented
Mar 7, 2016
OK #6346 is landing internally now, once it lands want to rebase this one on top of it because both require updating the |
Kureev
commented
Mar 8, 2016
ping me when I can rebase |
mkonicek
commented
Mar 8, 2016
Turns out #6346 doesn't work with our internal build based on Buck so we can probably merge this one first. |
mkonicek
commented
Mar 8, 2016
(oops wrong button 😃 ) |
mkonicek
commented
Mar 8, 2016
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 The root of the problem is we can't regenerate |
Kureev
commented
Mar 8, 2016
davidaurelio
commented
Mar 8, 2016
Can we get away with lodash 3? babel depends on that. In that case, |
davidaurelio
commented
Mar 8, 2016
no need to update the PR. I have it updated internally |
Kureev
commented
Mar 9, 2016
@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 |
grabbou
commented
Mar 9, 2016
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 ```  **Code formatting** Changes are aligned with the current [style guide](https://github.com/facebook/react-native/blob/master/CONTRIBUTING.md#style-guide). Closesreact#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 ```  **Code formatting** Changes are aligned with the current [style guide](https://github.com/facebook/react-native/blob/master/CONTRIBUTING.md#style-guide). Closesreact/react-native#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 ```  **Code formatting** Changes are aligned with the current [style guide](https://github.com/facebook/react-native/blob/master/CONTRIBUTING.md#style-guide). Closesreact/react-native#6030 Differential Revision: D3016271 Pulled By: davidaurelio fb-gh-sync-id: c4f6776a7de7470283d3ca5a8b56e423247f5e45 shipit-source-id: c4f6776a7de7470283d3ca5a8b56e423247f5e45

As far as we agreed to merge
rnpminto 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
Code formatting
Changes are aligned with the current style guide.