Unregister Services before they are registered

[MrChocolatine]

This should normally fix #413 .

@simonihmig

[changeset-bot]

🦋 Changeset detected

Latest commit: 71e831a

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[simonihmig]

Just to add some more context to this from an internal discussion for future travelers...

I think how this works with that service integration is that if a service is injected when that setupBrowserFakes test-helper hasn't been called yet, the service will be the "normal" one without any mocking capabilities. So using the real DOM APIs. And that's the timing issue that causes the bug. So with this PR, all service injections that happen after that call to
I think how this works with that service integration is that if a service is injected when that setupBrowserFakes will get the mockable service, while the one that got the service injected before that might still be using the real DOM APIs. Maybe that is enough to "fix" a particular test, when that test only interacts with the things that got the mockable service. But this seems still not be the right fix that is guaranteed to work everywhere.

However, as this is a very low-risk change, and it does no harm, and it does fix those cases where the services that needed to be mocked was only instantiated after calling setupBrowserFakes, I think we can land this for now, even if there is still some chance of the mock setup not working as expected in all cases.

/cc @NullVoxPopuli

[NullVoxPopuli]

I've only ever seen this problem come up when folks try to use services in initializers -- they should not 🙈

a safer alternative to initializers

In your application route, do your setup in one of the model hooks.
I like using beforeModel, like here https://ember-primitives.pages.dev/2-accessibility/index.md

Calling unregister does feel like it's more hiding the problem than fixing it. Ultimately, folks will want to fix whatever the underlying problem (or initializer) is.

[kellyselden]

Is there a failing test that can be made for this?

[MrChocolatine]

Do we move forward with this PR or do I patch again ember-browser-services in our internal codebase?

[ynotdraw]

It sounds like @NullVoxPopuli is thinking there's an underlying problem and this is an unreasonable patch? If so, it'd be best to resolve the root issue rather than patching an anti-pattern.

What do you think @simonihmig ?

[MrChocolatine]

Anti-pattern to unregister Services? What is this method good for in that case, when should it be used?

[NullVoxPopuli]

Anti-pattern to unregister Services?

When a service has already been used, yea -- because you're swapping the service with a different implementation than what your app/tests booted with.
And unregistering before first access is a no-op.

What is this method good for in that case, when should it be used?

I don't know 🤔

[MrChocolatine]

Anti-pattern to unregister Services?

When a service has already been used, yea -- because you're swapping the service with a different implementation than what your app/tests booted with. And unregistering before first access is a no-op.

register-ing Services already swaps them with the implementation provided during the call.
Then how come unregister-ing before register-ing is bad?

I am trying to understand.

[NullVoxPopuli]

register-ing Services already swaps them with the implementation provided during the call.

registering after a service has already been used though (should) error, iirc -- because you're changing the implementation already in use.

The only time it's safe to replace the service is if it hasn't already been used for your test/app instance (such as in an initializer).

I am trying to understand.

<3

[MrChocolatine]

I understand, thanks for taking the time to explain.

In that case, I don't know where we want to go with that PR, if there was/there is no alternative fix.
In the meantime I will probably bring back this patch to my work codebase as it does help to make many tests pass. :-\

[NullVoxPopuli]

it does help to make many tests pass. :-\

do you have any initializers accessing the browser services?

[MrChocolatine]

it does help to make many tests pass. :-\

do you have any initializers accessing the browser services?

I cannot tell for now, I need to take a deeper look (well, you kind of know the codebase I am talking about lol).

But also, I should have been more accurate when I said "it does help to make many tests pass":

• Running the entire test suite (add-on or app) – All tests pass
• Running a single test from that test suite, using the Rerun button – The test fails
  • Because a Service normally mocked by ember-browser-services is not

[simonihmig]

It sounds like @NullVoxPopuli is thinking there's an underlying problem and this is an unreasonable patch? If so, it'd be best to resolve the root issue rather than patching an anti-pattern.

Yes, I agree this is not the best fix, as you can see in my statement below. But I still think it does no harm compared to what we have now, and is better than nothing, until we come up with something better...

However, as this is a very low-risk change, and it does no harm, and it does fix those cases where the services that needed to be mocked was only instantiated after calling setupBrowserFakes, I think we can land this for now, even if there is still some chance of the mock setup not working as expected in all cases.

Is there a failing test that can be made for this?

Yeah, I think we should create one! Can do this later this week...

When a service has already been used, yea -- because you're swapping the service with a different implementation than what your app/tests booted with.

Yes, that's why the fix is not ideal. But after setupBrowserFakes has been called, all following injections see the mocked service. setupBrowserFakes being a no-op when something has accidentally already instantiated the service doesn't feel right either?

I've only ever seen this problem come up when folks try to use services in initializers -- they should not 🙈

I respect your opinion that you don't like this pattern, but I don't think we can say this is an agreed upon anti-pattern. The [initializer guide](https://guides.emberjs.com/release/applications/initializers/9 has lots of .lookup() examples (though no service). And in a code base we all here know very well, there are tons of places where an instance-initializer is looking up a service (think of service.register*()). So I think this addon must support that (or rather not interfere with that), no matter what.

do you have any initializers accessing the browser services?

I don't think it is directly doing that. Rather an initializer accessing a service that is accessing another injected service that has browser services injected and accessing some window thingy eagerly!
But again, I think that is valid. Nowhere seen any "official" statement that you should not do this.

<slightly-off-topic>
I know we (@NullVoxPopuli) talked about this many years ago IIRC, but I still don't fully grok why this addon takes the services approach. The Readme has this section on this addon vs. ember-window-mock, but I still don't get why this approach here should be preferred over the more simple one. On the pro side you could say that using services and DI allows you to replace a service with a different implementation (the main use case for DI), but I haven't seen where this ever happened. We always use the one implementation from this addon, and pass the things to be mocked to setupBrowserFakes(). But for that we don't need DI, right?

On the cons side though, and that's a thing that I have a hard time wrapping my head around, while modern Ember is about embracing the platform and less Emberisms, we are squeezing the platform feature of the window global into a very Ember-specific concept, that of services. And by that we are opting into that problem that we are discussing here. After all, ember-window-mock (which this is based on) does not suffer from this problem at all.
</slightly-off-topic>

I think it is possible to fix this properly by not even having to register the service explicitly, instead delegating to the window imported from ember-window-mock all the time (see #413 (comment)). Not sure if we need to be concerned about performance or something, because that will wrap the window global in a Proxy twice (one from ember-window-mock, to make it optionally mockable), and another from this addon to make it behave like a service.

[ynotdraw]

Approving to not be a blocker given Simon's response above. Adding a test, if possible, would be sweet!

[simonihmig]

Here is a reproduction: #428

Just added an instance-initializer looking up the service and... 💥

@MrChocolatine can you cherry pick that commit into your PR here to prove the issue is getting fixed please?

[MrChocolatine]

Here is a reproduction: #428

Just added an instance-initializer looking up the service and... 💥

@MrChocolatine can you cherry pick that commit into your PR here to prove the issue is getting fixed please?

Done @simonihmig .

[simonihmig]

Tests are passing, so the previously failing test is being fixed by this change. Will go ahead and merge this!