Removing the problematic share from the zone.js changes

[jamesdaniels]

I believe this to be the root cause of #2309, #2314, #2312.

This came up in the review of #2243 but neither my test applications or the test suite caught that it would cause any issues. I merged into 5.x series because I figured fromRef was deep enough in the codebase and dropping a share() wasn't a big deal.

I'm planning to drop this in a patch.

Demonstrating the break fix/here https://stackblitz.com/edit/angular-7kb1uf?file=src%2Fapp%2Fapp.component.ts

[sarunint]

I'm thinking shareReplay(1) might work here??

[jamesdaniels]

shareReplay(1) could certainly work, but that does have implications (memory) that I'm not sure if I want to take on in a minor.

[jamesdaniels]

Also share() really seems to just be a fix for the server block, perhaps we could shareReplay(1) only in the server env... but I think this needs to be thought out more.

[jamesdaniels]

Thoughts @kamshak? Hate to think I'm missing something here that you already thought out.

[jamesdaniels]

Cutting 5.4.2 and 6.0.0-rc.1 now with patches for this. Thanks for you help @sarunint, let's keep the discussion going and make sure we didn't break anything in SSR land.

[ValentinFunk]

@jamesdaniels looks good, sorry didn't run into this issue during testing. It should work fine without the share, it only helped when there were lots of subscriptions to the same (and nested) observables in SSR where sometimes it would cause the zone to block.

Maybe we can find a way to add tests for the issues reported (because of the share) and I'll try to reproduce the Zone issue it was supposed to fix in a test as well so we can safely work on a solution.

[jamesdaniels]

@kamshak no worries. I didn’t catch it either and you did the community a solid with that massive PR.

I agree some hot/cold/warm/lukewarm 😝 observable tests would be a good guard on future bugs of this sort. No rush though.

If you can repro the lock ups that would probably be most valuable. Would be nice to know at what scale things start acting up.

Also FYI I’m not opposed to a simple shareReplay so long as we’re refcounting.