dschmidt / ember-cli-deploy-sentry

An ember-cli-deploy-plugin to upload javascript sourcemaps to Sentry
MIT License
42 stars 51 forks source link

Apply the addon after `ember-cli-deploy-sentry` #60

Closed lolmaus closed 4 years ago

lolmaus commented 5 years ago

This part of the readme is confusing:

Install ember-cli-sentry but import the raven service from ember-cli-deploy-sentry/services/raven

I assume it means that the raven service must be defined in the app, where it reexports the ember-cli-deploy-sentry version of the service.

That part can be automated, which is implemented in this PR.

This PR also required an update to the readme, but a confirmation and guidance from addon maintainers is required to do this properly. I'm still confused.

dschmidt commented 5 years ago

Yes, you need to have both addons, because the service in this addon wraps the other one. Can you give me a little more explanation on what your change actually does?

lolmaus commented 5 years ago

Hey.

Both ember-cli-sentry and ember-cli-deploy-sentry define the raven service. I was concerned that the inclusion order of addons was not determined, causing the ember-cli-sentry one to win.

My workaround was to define an empty service by hand which makes no sense otherwise:

import {default as RavenDeployService} from 'ember-cli-deploy-sentry/services/raven';

export default class RavenService extends RavenDeployService {}

I've noticed that I did two mistakes in this PR.

  1. ember-cli-deploy-sentry does not put the service into the app namespace. So the inclusion order of addons is not important, a custom empty service is required anyway.
  2. I used the wrong addon name in package.json, should've been "after ember-cli-sentry".

I'll update the PR.

lolmaus commented 5 years ago

PR updated, please have another look.

lolmaus commented 5 years ago

TL/DR: this PR removes the necessity to define an empty raven service in the app.

dschmidt commented 4 years ago

I somehow completely lost track of this, mind rebasing? then I'll merge

lolmaus commented 4 years ago

@dschmidt Done.

dschmidt commented 4 years ago

still conflicting

lolmaus commented 4 years ago

image

dschmidt commented 4 years ago

Ah, well it cannot be rebased ... but it can be merged. :man_facepalming:

Having let you wait for so long, I'll stop nagging about that and just merge :)

dschmidt commented 4 years ago

Hm actually .. you added a yarn.lock file in the merge commit? Where is that from?

We already have package-lock.json, can't yarn deal with that?

lolmaus commented 4 years ago

yarn.lock removed. Sorry, my bad.

Note that you can squearge if you want one commit per feature:

image

dschmidt commented 4 years ago

I'm aware :-)

dschmidt commented 4 years ago

Finally merged - I'm super-sorry this took so long for something so trivial, thanks for your patience and being so quick now!