fix(router): resolve guard observables on the first emit

[awerlang]

Please check if the PR fulfills these requirements

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit-message-format
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
#9613

What is the new behavior?

On the first received value from an observable, decide to continue or cancel (de)activation. Waiting for completion is not required.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

I believe this isn't a breaking change, even though the behavior is changed (accepting more than one value in the stream), because this wasn't properly documented. Also, I think is much more convenient to not require the observable to complete.

[vsavkin]

Could you rebase the PR, so it can be merged into master?

[awerlang]

@vsavkin Done. Failing tests are unrelated, although I think router specs are not included in the passing ones, I'd double check that.
I kept use of the every operator to maintain the expectation of boolean values instead of any thruthy/falsy value, I guess that would be a breaking change, otherwise that could be simplified.

[syndicatedshannon]

Is there a pattern this implementation has in-mind, on how to disconnect when navigating away?

[awerlang]

@syndicatedshannon I don't have any idea what you're saying -- disconnect what?

[syndicatedshannon]

@awerlang I assume this causes a connection or subscription to the observable being resolved? When is it disconnected or unsubscribed?

If this implementation unsubscribes immediately, e.g. by simply calling "first", then if I understand correctly this is a breaking change and I would think it is also undesired behavior.

[syndicatedshannon]

export class ResolveFirst<T> implements Resolve<T> {
    constructor(public dataService: DataService, private resolverService: ResolverService) { }
    resolve(route: ActivatedRouteSnapshot, state: RouterStateSnapshot) { }

    resolveFirst(func: Function, param: string, route: ActivatedRouteSnapshot) {
        let obs = func.apply(this.dataService, [route.params[param]]);
        return this.waitForFirst(obs, route);
    }

    waitForFirst(obs: Observable<any>, route: ActivatedRouteSnapshot) {
        let connectedObs = obs.publishReplay(1);
        let sub = connectedObs.connect();
        this.connections.push(new RouteSub(this.buildUrlFromRoute(route), sub));
        return connectedObs.first().toPromise().then(_ => connectedObs);
    }

}

To clarify, here is a portion of our current implementation.

[awerlang]

then if I understand correctly this is a breaking change and I would think it is also undesired behavior

By returning an observable, you understand the framework needs to subscribe to it and later unsubscribe. Before this PR, users wouldn't call .unsubscribe() on their own code. This PR doesn't change that.
I can't agree to "it is also undesired behavior".

[syndicatedshannon]

How do you guard without subscribing?

[awerlang]

I can't see where this line of reasoning would lead us to. You haven't explained how this PR presents an undesired behavior. I saw you already open another issue, I guess it's better to move discussion there. Also, gitter could be a better channel for it.

[syndicatedshannon]

I'd be glad to clarify the undesired behavior, if any, if I understand the behavior.

Does the current implementation subscribe, retrieve the first item, and then unsubscribe?

[syndicatedshannon]

The expected behavior of an observable is that is a stream of data. That's what differentiates it from a promise.

It could be argued that a resolve on an observable should, by default, wait for the last item. I desire a more nuanced solution than that, but I can understand the difficulty in implementing it, and so I accept this "wait for completion/last item" behavior.

But keeping the expected behavior mentioned above in mind, I don't think it can be reasonably argued that a resolve on an observable should wait for the first item and then hide all access to future items.

I understand this may work well for an observable that has only one item. This is what we might see from a simple HTTP request. But that is simply represented by a promise, and missing (half of) the expected value of an observable.

Subscribing, retrieving the first value, and unsubscribing, requires the consumer to subscribe a second time in the controller, creating a duplicate network request.

[awerlang]

It could be argued that a resolve on an observable should, by default, wait for the last item. I desire a more nuanced solution than that, but I can understand the difficulty in implementing it, and so I accept this "wait for completion/last item" behavior.

Current approach lets user code use obs.takeLast().

But keeping the expected behavior mentioned above in mind, I don't think it can be reasonably argued that a resolve on an observable should wait for the first item and then hide all access to future items.

Oh wait! Did I go too far and implemented this for Resolve too? I meant this PR for OnActivate/OnDeactivate only. Can you confirm this?

I understand this may work well for an observable that has only one item. This is what we might see from a simple HTTP request. But that is simply represented by a promise, and missing the expected value of an observable.

Subscribing, retrieving the first value, and unsubscribing, requires the consumer to subscribe a second time in the controller, creating a duplicate network request.

This is valid for resolved data you want to access inside the component. Not for OnActivate/Deactivate data.

[syndicatedshannon]

Ahhh, I see! I don't know, I think I saw resolve linkage, but then I was expecting to see it, because of a comment someone left me on my other issue. Let me verify.

[syndicatedshannon]

Ugh, I'm going to need some time to clear my head with this. I've been working on 2.x for over a year and been through a lot of OnActivate implementations - it's messing with my head.

I don't initially see any relation to my resolve issue. I don't see immediately how expected behavior would be different for OnActivate "resolve", but since you pointed out the distinction I'm sure you are right.

Sorry for the misdirect.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}