adopted-ember-addons / ember-cli-flash

Simple, highly configurable flash messages for ember-cli
https://www.npmjs.com/package/ember-cli-flash
MIT License
355 stars 113 forks source link

Rewrite the services/flash-messages.js service #271

Closed izelnakri closed 5 years ago

izelnakri commented 6 years ago

Hi, I'm currently trying to use this addon with new ember-resolver(for module unification), in my new mber project. It was all going good until I run my tests. They were all passing before my mber migration but this time only some of my acceptance tests get flashMessages.danger method is not a function, flashMessages.success method is not a function errors. In some other tests these methods are called correctly.

So I started digging around the source code and saw that there is an internal this.registerTypes() call on each init: https://github.com/poteto/ember-cli-flash/blob/master/addon/services/flash-messages.js#L125

First of all I have to say this service code is very poorly written, it is very hard to follow along and I have read many messy source-code before:

I dont know where that flashMessageDefaults comes from, its not explicit in the source code, apparently this context is shared with something(possibly environment.js?) implicitly. Surely there is a definition in the environment.js and it gets injected to the Application in my tests entrypoint(tests/test-helper), but that is not the service context. Most of the code is 3 years old, surely it could be written better today, with less implicitness, I see many unit tests already for service.

I will keep debugging to see how I lose that implicitly injected flashMessageDefaults in some of the acceptance tests. It could also be possible that owner.registration and owner.lookups changes/restrictions in new ember-resolver causes this(there are some registrations in the initializers). Meanwhile, I highly suggest the maintainer to rewrite that service module once more.

izelnakri commented 6 years ago

The issue was probably in the ember version or the resolver Im using. I did the following two things and everything is working:

in my ember modules

export default Component.extend({
  flashMessages: service()
})

to:

export default Component.extend({
  flashMessages: service('flash-messages')
})

in test owner.lookups:

context.owner.lookup('service:flashMessages')

to:

context.owner.lookup('service:flash-messages')
Dhaulagiri commented 6 years ago

@izelnakri will https://github.com/poteto/ember-cli-flash/pull/284 solve the issue you were describing here?

olleolleolle commented 5 years ago

@izelnakri What do you think? Is this issue closable with the changes now in place?

sbatson5 commented 5 years ago

Closing. @izelnakri can bug me on our work slack if this is still an issue 😜