Remove lodash as a dependency #35

jonpitch · 2018-06-29T20:20:05Z

Fixes #34 by removing lodash and replacing with Ember's merge polyfill. I played it safe here instead of just using Object.assign, but feel free to modify depending on how many browsers you need to support.

Exelord · 2018-06-29T23:22:36Z

package.json

@@ -32,24 +31,24 @@
    "broccoli-asset-rev": "2.6.0",
    "ember-ajax": "3.0.0",
    "ember-bootstrap": "1.2.0",
-    "ember-cli": "~3.0.0-beta.2",
+    "ember-cli": "^3.1.4",


Please dont use ^ in packages.

Exelord · 2018-06-29T23:22:56Z

addon/services/rollbar.js

@@ -34,7 +34,7 @@ export default Service.extend({
  }).readOnly(),

  rollbarClient(customConfig = {}) {
-    let config = deepMerge({}, this.get('config'), customConfig);
+    let config = merge({}, this.get('config'), customConfig);


There is a big difference between merge and deepMerge :/ We definitely need the second one.

Exelord · 2018-06-29T23:24:24Z

package.json

@@ -23,7 +23,6 @@
  },
  "dependencies": {
    "ember-cli-babel": "6.11.0",
-    "ember-lodash": "4.18.0",


Maybe we can just use lodash.merge package and shim it.

Exelord · 2018-06-29T23:24:44Z

package.json

    "ember-cli-dependency-checker": "2.1.0",
    "ember-cli-eslint": "4.2.3",
    "ember-cli-fastboot": "1.1.3",
-    "ember-cli-favicon": "1.0.0-beta.4",
+    "ember-cli-favicon": "^1.0.0",


no upgrades please in this PR

jonpitch · 2018-06-30T12:39:23Z

@Exelord moving ember-lodash to a devDependency had the effect I was going for and the addon still seems to work as expected in my app. I came across this issue where it looks like I had an older version of broccoli-concat-analyser, so my initial report in #34 was off. The actual savings in my app is ~65k.

In regards to the deep merge, if that's needed for merging Rollbar configs, maybe some test coverage there would be good? I can add that; I just couldn't think of the use case.

Exelord · 2018-07-01T02:44:05Z

Can we move it to devDependencies? We are using it in production code, to it has to be compiled using this packages.

Yes we need that. If you'll look here https://github.com/Exelord/ember-rollbar-client/blob/master/config/environment.js
you'll see there is nested object. As we are allowing on overwriting we have to use deepMerge.
Otherwise having a config like:

{
  payload: {
    test: true,
  }
}

and passing in rollbar service:

{
  payload: {
    test2: true,
  }
}

will result:

{
  payload: {
    test2: true,
  }
}

which will overwrite the object and remove the test key

jonpitch · 2018-07-01T15:48:44Z

Thanks for the explanation @Exelord that was really helpful. I missed the part of the readme that specified users could create a new Rollbar instance, thus requiring the deep merge.

I added a test for that and brought in just lodash.merge. I also brought in ember-auto-import to make things a little easier. This is failing in CI; so I'm open to using a vendor-shim or something, but I'm less familiar with how that would work here.

We could always add our own implementation of a deep merge, something like this is probably sufficient and passes the test.

st-h · 2018-07-01T15:58:31Z

We could always add our own implementation of a deep merge, something like this is probably sufficient and passes the test.

haha, I was gonna write a merge function tonight, as that should be doable in just a few lines of code - given that we very likely do not need very complex logic to just merge the configs. Great you already found something that works.

Totally in favour of removing lodash though. I was wondering where that dependency comes from in our app a few days ago, but did not have the time to look further into it so far. Needless to say that it was significantly increasing the size of our app.

into no-lodash

jonpitch · 2018-07-26T13:16:04Z

@Exelord is there anything preventing this from being merged?

I also added a small change here to resolve the rollbar path a little differently. The way it was broke a mono-repo that I was using this dependency in. This should work in both cases, per this discussion.

Exelord · 2018-07-31T00:43:13Z

package.json

@@ -55,7 +54,7 @@
    "ember-maybe-import-regenerator": "^0.1.6",
    "ember-resolver": "4.5.0",
    "ember-scroll-to": "0.6.4",
-    "ember-source": "~3.0.0-beta.6",
+    "ember-source": "~2.12.0",


Why we downgraded?

tests/unit/services/rollbar-test.js

@@ -29,6 +30,27 @@ test('currentUser', function(assert) {
  assert.equal(service.get('notifier.options.payload.person.name'), 'User');
 });

+test('new rollbar client - deep merge config', function(assert) {


Exelord · 2018-07-31T00:44:56Z

addon/-private/deepMerge.js

@@ -0,0 +1,52 @@
+// from https://github.com/tchak/ember-fetch-adapter/blob/master/addon/-private/merge.js


This should be an util under addon/utils/deep-merge.js

And why we dont use here a lodash one?

Anyway we need full pack of tests for it

Exelord · 2018-07-31T00:45:31Z

addon/-private/deepMerge.js

@@ -0,0 +1,52 @@
+// from https://github.com/tchak/ember-fetch-adapter/blob/master/addon/-private/merge.js


And why we dont use here a lodash one?

Exelord · 2018-07-31T00:45:48Z

addon/-private/deepMerge.js

@@ -0,0 +1,52 @@
+// from https://github.com/tchak/ember-fetch-adapter/blob/master/addon/-private/merge.js


Anyway we need full pack of tests for it

jonpitch · 2018-07-31T17:11:51Z

@Exelord pr feedback addressed. Per your feedback, I reverted the deepMerge function in favor of using lodash.merge. To bring that module in, I opted for ember-cli-cjs-transform over ember-auto-import which adds 17kb to the vendor build. This does mean that consumers will get the cjs transform included though.

If you know of a better way to shim that in, let me know. I also left the deep merge config test, since you don't have tests anywhere else for that. If you'd like me to move it or add coverage, please help me by identifying some use cases of merging rollbar configs.

Exelord · 2018-08-26T03:08:08Z

Thank you! All looks good! There is just one thing. There is a difference between ember-cli-cjs-transform and ember-auto-import. Once is for transforming cjs modules to es6 and the second one allows you import dynamically node modules. Thats why in that case I would prefer to go with auto-import as cjs-transform is not needed here as this package already export es6 module.

jonpitch · 2018-08-26T20:58:46Z

@Exelord understood, i was hesitant to use ember-auto-import because it does add 17Kb to the build, which seems excessive to invoke a function. Is there any other way that you would be ok with?

Exelord · 2018-08-27T18:00:55Z

Well it depends if we want to still support ember 2.12. What do you think?

jonpitch · 2018-08-27T18:29:16Z

I think that's up to you, but I don't believe ember-auto-import supports back to 2.12.

Exelord · 2018-08-29T03:06:01Z

Ok I have a solution. Lets manually push it to vendor folder and import from there. The same as we do with rollbar. check index.js for more info :)

PoslinskiNet · 2018-08-29T05:16:15Z

I would vote for dropping 2.12 support. It's about time to do so (almost 3.4 is here).

Exelord · 2018-08-29T15:17:18Z

I think we should drop it when Ember-cli will do it, so that we can keep compatibility with everything supported by ember officially. It should be deprecated September 10th 2018 then we may drop it :)

PoslinskiNet · 2018-08-29T21:49:48Z

Agree. So few more days left and Lodash is completely gone :) ?

Exelord · 2018-08-29T22:23:31Z

Probably :) ... well Lodash will be still with us ;) ... but just one module

Exelord · 2018-08-29T22:24:31Z

package.json

    "git-repo-version": "1.0.2",
+    "lodash.merge": "^4.6.1",


Could you change ^ to ~?

Exelord · 2018-08-29T22:24:43Z

package.json

@@ -24,8 +24,9 @@
  "dependencies": {
    "broccoli-funnel": "^2.0.1",
    "ember-cli-babel": "6.11.0",
-    "ember-lodash": "4.18.0",
+    "ember-cli-cjs-transform": "^1.3.0",


Could you change ^ to ~?

@Exelord are we sticking with ember-cli-cjs-transform or going the vendor route? I pushed your recent feedback here, but if you want to go the vendor route, I'll need some more guidance there.

I'm not as familiar with broccoli trees; I follow bringing in the dependency through the vendor tree, but it's unclear how to import it in the service.

Exelord · 2018-08-29T22:25:53Z

addon/services/rollbar.js

@@ -2,7 +2,7 @@ import { getOwner } from '@ember/application';
 import { computed } from '@ember/object';
 import Service from '@ember/service';
 import Rollbar from 'rollbar';
-import deepMerge from 'lodash/merge';
+import merge from 'lodash.merge';


can we rename it to deepMerge? it's more meaningful to what it does.

Exelord · 2018-09-01T00:25:13Z

Perfect :) Lets wait then for Ember 2.12 deprecation

Exelord · 2018-09-06T18:57:35Z

I see in ember-cli 3.4 there is still support for 2.12 :/

st-h · 2018-09-07T17:55:53Z

can we please remove that lodash thing one way or the other? That dependency is large enough to make us switch to another rollbar client (or publish just another one )
I don't think dropping support for 2.12 will be a major issue. If one needs to use this addon within ember 2.12 the previous versions including lodash still are available. However, we could still use a custom implementation to merge the config if supporting 2.12 is crucial.

Exelord · 2018-09-07T19:56:19Z

Conflicts

into no-lodash

jonpitch · 2018-09-07T22:59:05Z

@Exelord any ideas on the CI failures?

Exelord · 2018-09-07T23:01:09Z

Try to remove this thing: importTransforms: require('ember-cli-cjs-transform').importTransforms, :/

jonpitch · 2018-09-08T12:13:36Z

@Exelord I think that's needed so we don't break consuming applications. There's an open issue about it here

All of the ember-try scenarios pass for me locally. Maybe you could clear the Travis cache and re-run the build?

Exelord · 2018-09-08T19:12:36Z

It looks like this issue is on travis or sth. I see its everywhere without any change on multiple projects
https://travis-ci.org/minutebase/ember-can/jobs/426134012

kimek · 2018-09-14T14:16:36Z

Any updates from travis ? I've check logs, configs, code and couldn't find any problems with it. Did you try run this again @Exelord ? Great job with this pull request btw !

Exelord · 2018-09-14T22:57:30Z

Restarted one more time. (clean cache)

Exelord · 2018-09-15T06:29:39Z

Eh... ok merging that. Tests are not related

kimek · 2018-09-25T18:11:33Z

is there a chance to make a release for it ?

Exelord · 2018-09-25T18:23:05Z

#40 This one should fix the tests

remove lodash dependency EmberExperts#34

b01e99c

Exelord requested changes Jun 29, 2018

View reviewed changes

jonpitch added 2 commits June 30, 2018 08:21

pr feedback

83c4ab3

fix import syntax

3664074

use lodash.merge

0f48273

jonpitch added 2 commits July 2, 2018 07:51

provide merge function

cad0d66

Merge branch 'master' of https://github.com/Exelord/ember-rollbar-client

7f320a3

into no-lodash

Exelord requested changes Jul 31, 2018

View reviewed changes

use lodash.merge EmberExperts#35

6bd25fc

Exelord requested changes Aug 29, 2018

View reviewed changes

pr feedback

bac583c

Exelord approved these changes Sep 1, 2018

View reviewed changes

Exelord approved these changes Sep 7, 2018

View reviewed changes

Merge branch 'master' of https://github.com/Exelord/ember-rollbar-client

9ad8819

into no-lodash

Exelord approved these changes Sep 7, 2018

View reviewed changes

update lock file

4746a16

Exelord merged commit 544b19b into EmberExperts:master Sep 15, 2018

		@@ -0,0 +1,52 @@
		// from https://github.com/tchak/ember-fetch-adapter/blob/master/addon/-private/merge.js

Remove lodash as a dependency #35

Remove lodash as a dependency #35

Conversation

jonpitch commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonpitch commented Jun 30, 2018

Exelord commented Jul 1, 2018 • edited Loading

jonpitch commented Jul 1, 2018 • edited Loading

st-h commented Jul 1, 2018

jonpitch commented Jul 26, 2018

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonpitch commented Jul 31, 2018

Exelord commented Aug 26, 2018

jonpitch commented Aug 26, 2018 • edited Loading

Exelord commented Aug 27, 2018

jonpitch commented Aug 27, 2018

Exelord commented Aug 29, 2018

PoslinskiNet commented Aug 29, 2018

Exelord commented Aug 29, 2018

PoslinskiNet commented Aug 29, 2018

Exelord commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Exelord commented Sep 1, 2018

Exelord commented Sep 6, 2018

st-h commented Sep 7, 2018

Exelord commented Sep 7, 2018

jonpitch commented Sep 7, 2018

Exelord commented Sep 7, 2018

jonpitch commented Sep 8, 2018

Exelord commented Sep 8, 2018 • edited Loading

kimek commented Sep 14, 2018

Exelord commented Sep 14, 2018

Exelord commented Sep 15, 2018

kimek commented Sep 25, 2018

Exelord commented Sep 25, 2018

Exelord commented Jul 1, 2018 •

edited

Loading

jonpitch commented Jul 1, 2018 •

edited

Loading

jonpitch commented Aug 26, 2018 •

edited

Loading

Exelord commented Sep 8, 2018 •

edited

Loading