Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove lodash #502

Merged
merged 7 commits into from
Jun 11, 2017
Merged

Conversation

Cryrivers
Copy link
Contributor

@Cryrivers Cryrivers commented Apr 27, 2017

Description

As functions imported from Lodash have been already supported by either ECMAScript 5 or Ember framework, this PR removes Lodash to reduce the addon's size.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@Cryrivers Cryrivers mentioned this pull request Apr 27, 2017
@Cryrivers
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@Cryrivers Cryrivers changed the title Remove Lodash Remove lodash Apr 27, 2017
@Cryrivers
Copy link
Contributor Author

Not sure what happened to Travis CI but I passed all test cases on my local machine.

@tsubik
Copy link
Contributor

tsubik commented Apr 27, 2017

Yeah, looks like something got flaky. I've run tests locally for your branch, and all looks fine 👍

@mileszim
Copy link

Lodash is giving the same deprecation warnings of Torii. Is it possible to fast-track this PR?

@jamesdaniels
Copy link
Contributor

I'm kicking this now with the switch to yarn, hopefully that helps the CI failures.

@jamesdaniels
Copy link
Contributor

Ack @Cryrivers, mind giving the failing tests a look now that we've locked it down with yarn?

@Cryrivers
Copy link
Contributor Author

image

@jamesdaniels Any idea why all test cases can pass on my local machine but not Travis CI?

@mileszim
Copy link

mileszim commented Jun 5, 2017

@Cryrivers I'm gonna take a gander it has to do with cached node_modules https://travis-ci.org/firebase/emberfire/jobs/239540498/config

Always do cleanups
@Cryrivers
Copy link
Contributor Author

@mileszim tried with no luck 😢

@ibroadfo
Copy link
Contributor

I tried correctly disabling caching by setting cache: false but the build still failed: https://travis-ci.org/ibroadfo/emberfire/jobs/241577735

@ibroadfo
Copy link
Contributor

Ah! it's because travis is using Firefox 31. I'm not sure exactly what code is causing the problem though.

@ibroadfo
Copy link
Contributor

I've submitted a PR that fixes the Firefox issue #516 and merged it into my copy of this lodash PR to check that it works, which it does! https://travis-ci.org/ibroadfo/emberfire/jobs/241726167 :)

@jamesdaniels jamesdaniels merged commit e0248ce into FirebaseExtended:master Jun 11, 2017
@jamesdaniels
Copy link
Contributor

Thanks!

@Cryrivers Cryrivers deleted the remove-lodash branch June 12, 2017 03:17
@mileszim
Copy link

nice work! thanks for getting this in!

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

Successfully merging this pull request may close these issues.

6 participants