Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perhaps Router Resolve shouldn't wait for Observable to Complete? #10556

Open
syndicatedshannon opened this Issue Aug 7, 2016 · 55 comments

Comments

@syndicatedshannon
Copy link

syndicatedshannon commented Aug 7, 2016

This is probably best classified as a Router feature request. I'll describe what it is, my thinking, and a use case.

What it is: Currently, the V3 Router waits for an Observable to Complete before completing navigation. I would like for it to instead continue navigation after the first Observable item is returned.

My thinking: Currently the Resolve treats the Observable as a Promise, defeating the power of the Observable. This doesn't present a barrier with Angular's Http, where only one item is returned. But unless I misunderstand, the point of using an Observable in Http is to allow easy extension throughout Angular into a scenario where the data is actually a stream.

Use case: I have a live data feed from a remote server. I subscribe to a resource, rather than get it. When viewing this resource in a page, I only need one snapshot to complete navigation and display the component, but I would still like the updates bound to the view.

More detail:

Observables as first-class citizens in Angular2 are great for displaying "live data". I currently rely on, and it is becoming more common to integrate, "live data" streamed/syndicated/published from web services. Regardless of the framework used to deliver streams to the client, Observables are the obvious choice to act as intermediary between the receiving agent/service and the view.

Resolvers in Angular are also very nice for a few reasons. One is that I can be sure I have all my data before performing initialization. This greatly simplifies my component initialization logic. It also simplifies managing built-in operations Angular performs, such as avoiding errors binding templates to properties on null models.

But where live data comes in, the current Resolve behavior is difficult to make use of. If we suppose the live data stream never completes, we have to instead resolve, for example, to Observable.first(), and throw away the remainder of the stream.

Or I can go through extra steps for each of my Observables, to Observable.publish etc. But that's not enough, because there's not a symmetric exit hook for resolve, that I see. So instead I have to do some extra magic, such as tear down resources I didn't initialize in the component.

IMO, in the spirit of Rx, a better behavior would be to:

  • Share the Observable, e.g. via PublishReplay
  • Connect
  • Subscribe
  • Wait for the first item
  • Unsubscribe
  • Complete navigation
  • Pass the shared Observable as ResolvedData
  • When exiting navigation Disconnect

Or something similar.

Your thoughts are appreciated. I'll also be happy to contribute effort if someone with design authority confirms an approach.

@syndicatedshannon syndicatedshannon changed the title Perhaps Angular Router shouldn't wait for Observable to Complete? Perhaps Router Resolve shouldn't wait for Observable to Complete? Aug 7, 2016

@alxhub alxhub added the comp: router label Aug 9, 2016

@johnchristopherjones

This comment has been minimized.

Copy link

johnchristopherjones commented Aug 17, 2016

I've actually been bashing my head against the wall for the last two days over this exact thing. I had no idea the observable had to complete in order for the navigation to complete. Retrospectively, I understand it where it came from—the promise based approach. Without live observables, it is proving to be difficult to merge the angular router with John Lindquist's course on building Redux-style Applications with Angular, RxJS, and ngrx/store.

It's also pretty unexpected. For example, this guide on RXJS says “[c]ompletion should not be taken as "event at the end of the program" or something. It's an event which may happen or not.”

Is the work-around something like return this.myobservable.take(1); from the DataResolver to return a cold observable or return Observable.of(this.myobservable); to return a hot observable?

@syndicatedshannon

This comment has been minimized.

Copy link
Author

syndicatedshannon commented Aug 26, 2016

@johnchristopherjones

Yes, although I wasn't thinking about this as a defect-inducing issue because we've moved past that, I know what you mean, and we did initially experience that confusion as well. Several times our team hit a multi-hour snag trying to track this same issue down. The first was me, the next two were the same team member hitting it twice, once after a refactor. Embarrassingly, we both still stared at it for a while the second time: As usual, locating a defect with no visible error can be time-consuming.

What is the workaround you are looking for? Yes, observable.take(1), a.k.a. observable.first() is how we initially hacked the issue. Unless I misunderstand your question, hot vs. cold not directly related.

However, observable.first() will throw away all the following data. That's only a problem if you care about the following data (which you probably do, since otherwise your observable would probably be "completed").

@syndicatedshannon

This comment has been minimized.

Copy link
Author

syndicatedshannon commented Aug 26, 2016

@johnchristopherjones : Side note, I didn't see your question earlier. If you added it in an edit, you may want to post it separately next time.

@kemsky

This comment has been minimized.

Copy link

kemsky commented Sep 9, 2016

This is good idea, but i see at least two problems:

  1. once router stops waiting for observable to complete we will loose possibility to do redirect before component is created. I would suggest to make Resolve configurable.
  2. we need to unsubscribe at some point and it gets tricky when components are reused by the router.
@DzmitryShylovich

This comment has been minimized.

Copy link
Contributor

DzmitryShylovich commented Nov 6, 2016

I believe this was fixed #10412

@syndicatedshannon

This comment has been minimized.

Copy link
Author

syndicatedshannon commented Nov 7, 2016

Thank you for the find/reference, Dzmitry.

I raised a specific issue in my "desired behavior":

  • When exiting navigation Disconnect

Is there a pattern in-mind, on how disconnects are handled now, after #10412 ?

@syndicatedshannon

This comment has been minimized.

Copy link
Author

syndicatedshannon commented Nov 7, 2016

@DzmitryShylovich After an errant discussion on #10412, it looks like although it is a related behavior, it has no effect on this issue.

That PR is intended to change the behavior of guards (OnActivate), not Resolve.

@swftvsn

This comment has been minimized.

Copy link

swftvsn commented Oct 27, 2017

Router 4 should really support hot routes, that is views that are updated once the observable emits new data.

The activated route should flow the data and if route change is needed it should happen after the observable has emitted the first value.

The disconnect to the resolve observable should happen when the route is not activated anymore.

@greggbjensen

This comment has been minimized.

Copy link

greggbjensen commented Feb 8, 2018

I have this same problem. I used a Resolve with a Promise for an Observable to work around it. My service in this case caches the data. It would be nice if this was supported natively.

@Injectable()
export class DocumentsResolve implements Resolve<Promise<Observable<IDocument[]>>> {
    constructor(
        private _documentService: DocumentService,
    ) {}

    public resolve(
            route: ActivatedRouteSnapshot,
            state: RouterStateSnapshot): Promise<Observable<IDocument[]>> {

        return new Promise((resolve, reject) => {
            const documentObservable = this._documentService
                .list();

            documentObservable
                .first()
                .subscribe(() => resolve(documentObservable));
        });
    }
}
@Component({
    selector: 'app-document-library',
    templateUrl: './document-library.component.html',
    styleUrls: ['./document-library.component.css']
})
export class DocumentLibraryComponent implements OnInit, OnDestroy {
    public documents: Observable<IDocument[]>;

    constructor(
        private _route: ActivatedRoute,
    ) {}

    public ngOnInit(): void {
        this.documents = this._route.snapshot.data['documents'];
    }
}
@greggbjensen

This comment has been minimized.

Copy link

greggbjensen commented Feb 8, 2018

Here is an example of simplifying above with a re-usable base class.

export abstract class HotResolve<T extends Observable<any>> implements Resolve<Promise<T>> {
    public resolve(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Promise<T> {
        return new Promise((resolve, reject) => {
            const observable = this.hotResolve(route, state);
            observable.first().subscribe(() => resolve(observable));
        }
    }

    public abstract hotResolve(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): T;
}
@Injectable()
export class DocumentsResolve extends HotResolve<Observable<IDocument[]>> {
    constructor(
        private _documentService: DocumentService
    ) {
        super();
    }

    public hotResolve(
        route: ActivatedRouteSnapshot,
        state: RouterStateSnapshot): Observable<IDocument[]> {

        return this._documentService.list();
    }
}
@Rodrigo54

This comment has been minimized.

Copy link

Rodrigo54 commented Feb 11, 2018

A good example for using this feature is the own angularfire2 - Firestore that work with real-time data updates. I would like to continue to note the updates to the resolved route data.

@tatsujb

This comment has been minimized.

Copy link

tatsujb commented Feb 14, 2018

hey @greggbjensen , in your example (second version) what must DocumentService be?

Is it calling the service that I'm failing to get past navigation or is it vice versa and I'm supposed to call and use DocumentResolve with my service, if so what's DocumentService, I don't get how to use your code.

also : "ERROR in src/assets/services/hot.resolve.service.ts(8,18): error TS2339: Property 'first' does not exist on 'Subject' ."

@syndicatedshannon

This comment has been minimized.

Copy link
Author

syndicatedshannon commented Feb 17, 2018

@greggbjensen : Actually I think it takes more boilerplate code than that. You might have it already, under the covers elsewhere, but without it, here's what probably happens:

  1. observable is created
  2. observable is subscribed
  3. network request is made
  4. first item arrives
  5. subscription is released
  6. observable is provided to component
  7. observable is subscribed
  8. network request is made
  9. component is constructed with no values pending on observable stream (as though not resolved at all)

Note the network request is made twice, and yet navigation still completes without data sequenced on the observable stream.

That's the reason for the reference to "PublishReplay" in my original comment. Years later, we are still fighting this issue pretty severely. We've iterated through many solutions, all of which are far from perfect due to lack of support for this need at various points in router code. For example, another problem is "where is the best place to unsubscribe?", which is why I posed this question on S/O. Yet another problem is a maintainable pattern for defining resolvers for this.

If you like, I'll try to dig up some of our current base classes, but as I mentioned, none of it is pretty (not for lack of trying).

@syndicatedshannon

This comment has been minimized.

Copy link
Author

syndicatedshannon commented Feb 17, 2018

Here's the core code for our current implementation relative to this topic:

export abstract class ViewModelFactory implements Resolve<ViewModel> {
	constructor(private router: Router) { }

	// could possibly implement a shareReplay handoff, instead of this publishReplay connection manager,
	// by monitoring ActivationEnd, which holds the ActivatedRouteSnapshot to directly unsubscribe or disconnect
	resolve(route: ActivatedRouteSnapshot, router: RouterStateSnapshot) {
		let vm = RouteValue.useActiveProvider<ViewModelFactory>(route).value(this, (k) => route.paramMap.get(k));
		this.uponLeaving(route, () => vm.disconnect());
		return vm.connect();
	}

	private uponLeaving(route: ActivatedRouteSnapshot, action: () => void) {
		// TODO: This doesn't account for query parameters due to inaccessible UrlTree ctors
		let url = [""].concat(...route.pathFromRoot
			.filter(r => r.url instanceof Array)
			.map(r => r.url.map(s => s.path)))
			.join('/');
		this.router.events
			.filter(e => (
				e instanceof NavigationEnd ||
				e instanceof NavigationCancel ||
				e instanceof NavigationError
			) && !(this.router.isActive(url, false)))
			.first()
			.subscribe(action);
	}
}

export abstract class ViewModel {
	subscription = new Subscription();
	private hasConnected = false;

	connect() {
		if (this.hasConnected) {
			return;
		}
		this.hasConnected = true;
		let observables = Object.keys(this)
			.filter(k => this[k] instanceof Observable)
			.map(k => this[k] = (this[k] as Observable<any>).publishReplay(1));
		observables.forEach(o => this.subscription.add(o.connect()));
		return Observable.combineLatest(observables).first().map(_ => this);
	}

	disconnect() {
		if (this.subscription) this.subscription.unsubscribe;
	}
}

@Injectable()
export class AppSpecificViewModelFactory extends ViewModelFactory {
	// this factory provides a receiver for injected services we require,
	// without needing a distinct class for each one-line network call
	constructor(private ds: DataService, router: Router) {
		super(router);
	}

	metric(metricId: number) {
		return new VM.MetricViewModel(metricId, this.ds);
	}

	metrics() {
		return new VM.MetricsViewModel(this.ds);
	}
}

export class MetricViewModel extends ViewModel {
	// we can't resolve this from router without an intermediary,
	// unless we distribute matching magic strings (in this case 'metricId') through multiple files
	constructor(public id: number, private ds: DataService) { super(); }
	metric = this.ds.metric.getMetric(this.id);
	metricFeedback = this.ds.metricFeedback.getForMetric(this.id);
	metricCriteria = this.ds.metricCriteria.getForMetric(this.id);
}

export routes = TransformViewModelRoutes(AppSpecificViewModelFactory, [{
	path: ':metricId',
	component: MetricComponent,
	// this arrow expression breaks AoT, and necessitates a pre-compile step.
	viewModel: (r, p) => r.metric(+p("metricId")),
	// the following resolver is implied by the transform above
	// resolve: AppSpecificViewModelFactory
}]);
@syndicatedshannon

This comment has been minimized.

Copy link
Author

syndicatedshannon commented Feb 19, 2018

Note that the above implementation has some hooks to solve other router issues we've encountered as well, such as distributing magic string parameters, type-safe resolves, constructing an UrlTree outside of the router, type-safe parent route parameters, and others. We haven't resolved pattern issues surrounding AoT and related issues. So much to say, but honestly we're all kinda burned out on this issue.

@syndicatedshannon

This comment has been minimized.

Copy link
Author

syndicatedshannon commented Feb 20, 2018

@jasonaden : p.s. I just added a demonstration of where this issue is causing us a lot of grief. It also shows a couple other related issue we are trying to solve. I'd love it if you would take a look and give a little guidance, if you see better ways to accomplish any of this. We've been fighting this for a long time and could use some author guidance.

@syndicatedshannon

This comment has been minimized.

Copy link
Author

syndicatedshannon commented Feb 20, 2018

@kemsky :
"1. once router stops waiting for observable to complete we will loose possibility to do redirect before component is created." - I don't see this the same way. The current assumption is that there is only one item on observable sequence. Therefore, proceeding with navigation after the first result doesn't break most implementations. I'm not suggesting allowing configuration wouldn't capture any use-cases, it just would not be relevant to most. Also, even without global configuration, returning the observable as a promise, returning observable.last, observable.reduce, or any similar operators, all provide an easy "local configuration" approach. Selecting an observable operator is necessary anyway, to determine what is actually done with multiple stream values.

"2. we need to unsubscribe at some point and it gets tricky when components are reused by the router" - totally agree. that's why this is so hard. you can see publishReplay and shareReplay used to address this in my sample above, however it becomes harder because UrlTree constructors and other related tooling isn't exported for public use. capturing query parameters would address the component reuse scenario, but it might be more easily addressed by integrating other internal router tooling instead.

@jasonaden

This comment has been minimized.

Copy link
Contributor

jasonaden commented Feb 22, 2018

@syndicatedshannon Understood. I definitely agree this is an issue. Right now we're preparing for v6 RC.0, which means getting a few critical fixes in plus some updates to support the new rxjs.

That being said, shortly after RC.0 starts, I'll be diving in on this topic plus others with regards to the router and hopefully directly addressing how Observables are exposed/used within the router.

I know it's been a while on this topic. Part of the problem is not having consistency across the framework in terms of how Observables are exposed and used. But this is something we're looking to standardize on, and it will likely start with the Router.

@greggbjensen

This comment has been minimized.

Copy link

greggbjensen commented Feb 22, 2018

@tatsujb and @syndicatedshannon, here is an example of the DocumentService. It holds onto the original Observable for the request that was made. It then uses fetch to get the list again and publish back through the same stream, when an update, create, or delete is made.

Any component that calls list() will get the cached observable and the same stream that updates.

// Cache for hot observable that allows stream to be cached and pushed.
export class SubjectFetch<T> {
    private _subject: ReplaySubject<T>;
    private _observable: Observable<T>;
    private _fetch: () => Observable<T>;

    constructor(fetch: () => Observable<T>) {
        this._subject = new ReplaySubject<T>();
        this._observable = this._subject.asObservable();
        this._fetch = fetch;
    }

    public fetch(aggregate: boolean = true): void {
        this._fetch()
            .first()   // Prevent the need to unsubscribe.
            .subscribe(value => this._subject.next(value));
    }

    public get observable(): Observable<T> {
        return this._observable;
    }
}

@Injectable()
export class DocumentService {
    private _documentsCache: SubjectFetch<IDocument[]>;

    constructor(private _http: HttpClient) { }

    public list(): Observable<IDocument[]> {

        // Use cache if we have it.
        if (!this._documentsCache) {
            this._documentsCache = new SubjectFetch(() =>
                this._http.get<IDocument[]>(`http://somedomain.com/api/documents`));
        }

        return this._documentsCache.observable;
    }

    public create(document: IDocument): Observable<IDocument> {
        const result = this._http.post<IDocument>(
            `http://somedomain.com/api/documents`, document);

        // Fetch documents list again and publish back through hot observable.
        result
            .first()   // Prevent the need to unsubscribe.
            .subscribe(() => this._documentsCache.fetch());

        return result;
    }
}
@syndicatedshannon

This comment has been minimized.

Copy link
Author

syndicatedshannon commented Feb 23, 2018

@greggbjensen : do your observables all end themselves? I don't see the subscription token saved anywhere? Sorry, I'm not really following how this works. It's not critical, as I think I get the general idea relative to this ticket.

@tatsujb

This comment has been minimized.

Copy link

tatsujb commented Feb 23, 2018

@syndicatedshannon As far as I know on services and injectables you don't need to unsub since they handle this themselves. Unsubbing is a pattern reserved for components.

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Nov 18, 2018

Observables are a fundamentally different concept from promises, and promises don't have any distinction of "emission" and "completion".

@Airblader That's true, and my conclusion is:

Resolve (or any other Guard) shouldn't support Observable at all.

All of these issue are simply due to concept of Observable not fit into concept of Guard. The role of a Guard is only wait for a possible-async task to complete, the phasing is easy to understand in RouterEvent:

There's never a temporal dynamic binding in Guard, making Observable conceptually a footgun in first place. Also all of these task indicators are function return values, not shared properties, the definition of start is already determined by the invocation of the function, making an extra SUBSCRIPTION phasing also being nonsense.

Making Observable acceptable simply because they've been used a lot isn't a legit approach, the same issue apply to loadChildren, it's ridiculous to have a () => Observable interface which simply duplicates the definition of start, besides the temporal emission totally not making sense for code-splitting.

Having first-class support of Observable in Angular is great, but abusing it regardless of the semantics would be absolutely harmful.

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 18, 2018

I see your argument, but I disagree because I think the 99% usecase of a guard is a simple HTTP call, which returns an observable due to the HttpClient design (where observables really are the correct choice), so forcing users to convert this to a Promise all the time is just cumbersome. So to me it's logical to return an observable from the guard.

I also think it's perfectly reasonable if the guard waits for the first emission rather than completion, I don't have a preference for that.

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Nov 18, 2018

but I disagree because I think the 99% usecase of a guard is a simple HTTP call, which returns an observable due to the HttpClient design

It would simply break when having the wrong observe or reportProgress, as an HTTP Observable doesn’t have to be single-value, making this approach error-prone.

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 18, 2018

I don't really understand what you're saying? A http call that emits multiple times is also a rare thing, and a user would probably know when they are doing that. And progress events are even less likely to be relevant for the result of a guard, so you'd filter them out in any case.

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Nov 18, 2018

A http call that emits multiple times is also a rare thing, and a user would probably know when they are doing that.

That’s why requiring complete being acceptable while taking the first isn’t. If you’re not making multiple-value observable than current requirement won’t affect your use case. But it’s not acceptable to make others to non-debuggable error as they’re using the API correctly.

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 18, 2018

It sounds to me like you're arguing that the correct API should be waiting for the completion, or in other words that you think the current behavior shouldn't be changed at all (which is fine with me).

For completeness sake, though, if guards would use the first emission of an observable, you could still just design your observable to filter out any emission that aren't actually the result you want to give. I think that's perfectly reasonable. At the end of the day, the guard wants one value and one value only, so whether it uses the first or last emission of an observable for that is purely a design choice and doesn't prevent any particular usecase from working.

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Nov 18, 2018

you could still just design your observable to filter out any emission that aren't actually the result you want to give. the guard wants one value and one value only

Making explicit validation of emit times (throwing error on multiple emits) is also reasonable to me, having a must-only-emit-one-value requirement is valid, as long as it’s forced by framework (rather than relying on user). That’s also why not waiting for complete isn’t valid, would even break in a startWith operator which is common practice in async programming.

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 18, 2018

The framework cannot enforce that. If we decided to use the first emission, we'd logically unsubscribe from it once we saw that value, thus never knowing whether a second value would have been emitted or not.

It doesn't matter if guards use the first or last value: the user has to be careful either way. And either way, the framework cannot ensure correct usage or give warnings.

If we wait for completion (current behavior), the user has to ensure that the observable completes. That's what spawned this issue in the first place.

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Nov 18, 2018

The framework cannot enforce that.

That’s why waiting for complete must be happening.

If we wait for completion (current behavior), the user has to ensure that the observable completes.

It can be handled by a ROUTER_GUARD_TIMEOUT configuration, thus it would be forced by framework rather than relying on user, I didn’t mean current forever-waiting strategy is the right one.

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 18, 2018

Perhaps a timeout that in dev-mode only prints a warning message to the console would be useful, but in production mode this timeout needs to be disabled for sure. The framework cannot dictate how long the guard is allowed to take.

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Nov 18, 2018

I agree, it might be made an option like enableTracing and controlled by environment.

@pickfire

This comment has been minimized.

Copy link
Contributor

pickfire commented Nov 19, 2018

If we wait for completion (current behavior), the user has to ensure that the observable completes. That's what spawned this issue in the first place.

I believe the user only know about that when they search for this issue online or that they found this thread, this is not easily discovered by the users and is not checked during compile time.

Perhaps a timeout that in dev-mode only prints a warning message to the console would be useful, but in production mode this timeout needs to be disabled for sure. The framework cannot dictate how long the guard is allowed to take.

Looks interesting, it might be something like the dev-mode warning in the console. I bet this could avoid a lot of users from getting into this pitfall by default.

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 19, 2018

I believe the user only know about that when they search for this issue online or that they found this thread, this is not easily discovered by the users and is not checked during compile time.

Yes, because it cannot be checked. The framework can never know whether the user forgot to complete the observable or whether the completion will still happen.

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Nov 20, 2018

I believe the user only know about that when they search for this issue online or that they found this thread, this is not easily discovered by the users and is not checked during compile time.

Yes, from my feeling avoid implicit failure (unexpected behavior without failing) is the first-class requirement, which should be ensured in any path.

Also, wrong timing (progressive) could be even more worse than not finishing (yes/no), so I'd suggest it always been covered by runtime check rather than relying on user reading the documentation carefully, or simply drop the support for Observable if the API fails to be intuitive.

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 20, 2018

simply drop the support for Observable if the API fails to be intuitive.

Killing the 99% usecase is a bad idea in my eyes. If a simple return this.http.get() no longer works, don't you think there'd be a constant stream of users asking "why isn't it working", "how can I make it work", "why can't I return an observable"?

If we want to deal with the problem that users don't understand how observables work, we should switch from waiting for completion to waiting for first emission. This should again be the solution that solves the majority of the problems here, as generally that is what the user wants to happen.

Note that either way this is a breaking change and with Angular being committed to avoiding these the benefit of the breaking change should be weighed against the cost of it. Removing observable support would break almost all guards in existence — that doesn't sound like a good idea to me.

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Nov 20, 2018

If a simple return this.http.get() no longer works, don't you think there'd be a constant stream of users asking "why isn't it working", "how can I make it work", "why can't I return an observable"?

First, I don't think it could happen. If this is going to happen, then it would only be deprecated first at MAJOR VERSION + 1, and everyone could receive a warning message. And then in MAJOR VERSION + 3 it could be removed.

If we want to deal with the problem that users don't understand how observables work, we should switch from waiting for completion to waiting for first emission.

With those people don't understand it, how do you know what they really wants? If only first emission should be used, then only one emission should happen (thus must wait for complete), that's a good restriction which could help people understand how guard works.

Note that either way this is a breaking change and with Angular being committed to avoiding these the benefit of the breaking change should be weighed against the cost of it.

The request itself is also a breaking change which break application silently, but removing observable (even make it breaking way) would produce an explicit error message at very early stage (compilation) which is much better.

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 20, 2018

With those people don't understand it, how do you know what they really wants?

My personal experience, so admittedly this is biased. :-) I've seen this issue come up with coworkers and on SO and I don't think even once did the user want something other than use the first emission for the guard.

This issue mostly arises when using either switchMap from a non-completing observable to an HTTP call (get current user, request auth status) or when using forkJoin on a non-completing observable. In the first case, the first emission is what the user wants, the second case wouldn't be covered by any of the solutions discussed here.

What would be the usecase for an observable that emits more than once but where you want to use the last emission?

If only first emission should be used, then only one emission should happen (thus must wait for complete)

I'm sorry, I'm not following what solution you're arguing for anymore. :-) This is the current behavior. The framework cannot know whether an observable will complete in the future — it's just conceptually not possible, because it'd require time travel.

that's a good restriction which could help people understand how guard works.

That's not a restriction, that is the current behavior. How does the current behavior help people understand how guards work?

The request itself is also a breaking change which break application silently, but removing observable (even make it breaking way) would produce an explicit error message at very early stage (compilation) which is much better.

I disagree that you make an API more intuitive by making it more restrictive. A good API accepts any input which it can reasonably understand. That's the most user-friendly as it requires them to worry less about what they have to return — it just works, in the majority of cases.

Observables and promises are both forms of input the guard can make sense of.

By removing observable support from guards you're eliminating this particular issue by introducing the broader issue of people not knowing how they can make HTTP calls to implement a guard while forcing all other users to convert all their guards to promises for no apparent reason (they'll never see this discussion).

@swftvsn

This comment has been minimized.

Copy link

swftvsn commented Nov 20, 2018

I still don't get why the router does not support hot routes, that is, routing is done for each emit. IF the route is already correct one, then only the route data is pushed to the already visible view.

All current guards etc. can be called - if the new emit changes something, the guard must be run again to see if the user still has permission to view etc.

And the disconnect, or unsubscribe would happen once the route ceases to be active.

@swftvsn

This comment has been minimized.

Copy link

swftvsn commented Nov 20, 2018

The model I describe in previous comment works the same for the completable / http / promise based stuff I think, so no compatibility or warnings would be needed either. It would only enable the framework to listen to the observable if there is additional data down the road and pass that to the view after guard checks.

It would just work.

If the observable is still hot (not completed) when the next route change happens, it would be unsubscribed from.

This, of course, is the high level developer experience I would like to have not knowing all the gory details about the current implementation + all the supported corner cases..

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 20, 2018

Personally, I very much like that idea. It would make it possible to have "real-time updates" to the authentication status as as soon as the status changes, the guard would do its job even if the route could previously be activated.

However, I think it might make sense to implement this as a different, new, guard (canRemainActive or something similar) as canActivate is more concerned with activating the route initially.

Maybe file a separate feature request?

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 20, 2018

Actually scratch the part about a separate guard. I think it'd be more beneficial to have it in CanActivate directly as it avoids some weird questions and solved this issue as well as you described.

That said, I don't know the internals either and can imagine that this raises questions about the order of guards etc.

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Nov 20, 2018

This issue mostly arises when using either switchMap from a non-completing observable to an HTTP call (get current user, request auth status).

This is already a quite dangerous scenario where there could be competing pending request (although old one will be cancelled) and it's not clear which one will be used for guard, in a specific application one may have knowledge of some requests being idempotent, but a library shouldn't make that conclusion.

What would be the usecase for an observable that emits more than once but where you want to use the last emission?

Anything with a pending state (common when used in view), like:

// will get { id: '1234' }
this.auth().pipe(
  startWith({ id: null })
)

I'm sorry, I'm not following what solution you're arguing for anymore. :-) This is the current behavior.

This is not about using last emit, but count the emission and report error when it happened more than once.

That's not a restriction, that is the current behavior.

Again.

That's the most user-friendly as it requires them to worry less about what they have to return — it just works, in the majority of cases.

A good API always works in any of the type-compatible inputs, rather than having additional semantic requirements, that's why Observable should not supported as it doesn't support ever-changing value. But it's hard to change due to already supported, hence the discussion.

Observables and promises are both forms of input the guard can make sense of.

It doesn't make sense unless guard support any of the emission (not first one nor last one).

By removing observable support from guards you're eliminating this particular issue by introducing the broader issue of people not knowing how they can make HTTP calls to implement a guard while forcing all other users to convert all their guards to promises for no apparent reason

If someone don't know how to convert Observable to Promise, I'd suggest them don't use Observable, it would only lead to more confusion when they do.

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 20, 2018

This is already a quite dangerous scenario where there could be competing pending request (although old one will be cancelled) and it's not clear which one will be used for guard, in a specific application one may have knowledge of some requests being idempotent, but a library shouldn't make that conclusion.

There's no pending request danger (that's what switchMap guarantees) and the framework doesn't have to make any conclusion here. Many guards will look exactly like that:

return this.userService.currentUser$.pipe(switchMap(user => this.authService.isAuthenticatedForResource(user)));

I don't see any issue with such an implementation other than the user may just have to add a first() due to the fact that otherwise this doesn't complete.

This is not about using last emit, but count the emission and report error when it happened more than once.

OK, so your suggestion is to keep the behavior as-is, but just log a warning if there has been more than one emission? Yeah, that makes sense to me.

If someone don't know how to convert Observable to Promise, I'd suggest them don't use Observable, it would only lead to more confusion when they do.

The same argument could be made for the issue itself here: if you don't understand the difference between emission and completion and that you need to complete your observable, don't use observables. It's a fair argument, but not a newbie-friendly one. :-) Angular beginners don't usually "choose" to use observables, the HttpClient more or less forces it on them.

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 20, 2018

Maybe just as a recap, I think we have the following proposals on the table, right?

  1. In dev mode, log a warning if the observable returned from the guard hasn't completed within a certain period of time.
  2. In dev mode, log a warning if the observable returned from the guard has emitted more than once upon completion.
  3. Change existing behavior to use the first emission rather than waiting for completion.
  4. Remove observable support from guards
  5. Add support for multiple emissions in the router by actually respecting each emission until the route is deactivated.

My 2c: I think #1 and #2 can both be helpful, and are non-breaking dev-only additions. #3#5 are actual changes, of which I'd prefer #5, then #3, and very much dislike #4. :-)

@swftvsn

This comment has been minimized.

Copy link

swftvsn commented Nov 20, 2018

Thanks for the recap @Airblader . One vote for option 5.

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Nov 20, 2018

There's no pending request danger (that's what switchMap guarantees) and the framework doesn't have to make any conclusion here.

Consider the timing:

  • The guard is invoked and the Observable above is passed to router;
  • The first HTTP request being send;
  • currentUser$ emits a new value;
  • The first HTTP request being cancelled and second HTTP request be made;
  • The second HTTP response received;
  • Guard check the second HTTP response result.

So that what being checked against is not the user when guard being invoked. In the application there might be no chance for currentUser$ to change at that time, but it's about detailed business logic, from the usage it's indeed a dangerous approach.

OK, so your suggestion is to keep the behavior as-is, but just log a warning if there has been more than one emission?

Many options are reasonable to me, except the take the first value and then unsubscribe one, thanks for listing above.

Imagine an API with:

function process(arr: number[]): void {
  const value = arr[0]
  register(value)
}

Then I'd say it's definitely not intuitive, it would either:

  • Refine the logic and make use of each value;
  • Not accept array, instead a plain value;
  • Switching the type of arr from number[] to [number];

Back to the issue here, Observable and Promise are just temporal equivalents of Array and value, thus the options can be inferred. But there's no Array/Tuple typing different for Observable, so it cannot be made a compile-time check but runtime.

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 20, 2018

So that what being checked against is not the user when guard being invoked. In the application there might be no chance for currentUser$ to change at that time, but it's about detailed business logic, from the usage it's indeed a dangerous approach.

Point taken, I see now what you mean with how that causes the framework to make a decision it shouldn't be making.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.