Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(ngInclude): emit $includeContentError when HTTP request fails #5825

Closed
wants to merge 1 commit into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Jan 15, 2014

This adds a scope event notification when a template fails to load.

This can have performance implications, and unfortunately cannot at this moment be terminated with preventDefault(). But it's nice to be notified when problems occur!

Closes #5803

@btford btford added the gh: PR label Mar 25, 2014
@IgorMinar
Copy link
Contributor

lgtm

@shahata
Copy link
Contributor

shahata commented Apr 17, 2014

wouldn't it make sense to have attr.onerror here as well, even if only for the sake of symmetry with the existing attr.onload?

@shahata
Copy link
Contributor

shahata commented Apr 17, 2014

Also, do we really want to report an error even if thisChangeId !== changeCounter? This is also not symmetric with the current successful load behavior...

@caitp
Copy link
Contributor Author

caitp commented Apr 17, 2014

@shahata re: the onerror thing, I'm not totally opposed to it, but I think it should wait until people ask for it at the very earliest. Even this event is probably a bit much, but it's been asked for a bit.

As for emitting the error even if it wasn't for the same change counter, I'm not sure it really makes a huge difference, because in practice people aren't changing the expression and running a digest 50 times before the response arrives. But it's a small change to make it work like that, so I guess that's fine.

@cdaringe
Copy link

Thanks cait. Looking forward to this merge

@caitp
Copy link
Contributor Author

caitp commented May 13, 2014

Yeah, this should be pretty trivial, @petebacondarwin or @matsko can you give this a quick review and see if @shahata's concern needs to be addressed? I'll talk to Igor about merging this on Thursday

@petebacondarwin
Copy link
Contributor

@caitp I agree with you that we should wait on adding onerror until it is requested. It will be a small simple addition later.
@shahata - I agree with you that we should not bother emitting the error if the change counters do not match since the fact that they don't match means that this response is out of date and so it could be that a subsequent request has succeeded and everything is OK, so emitting an error at that point would be misleading.

@petebacondarwin
Copy link
Contributor

Other than that LGTM

@IgorMinar
Copy link
Contributor

yeah, we should emit the error only if the error is for the current request

This adds a scope event notification when a template fails to load.

This can have performance implications, and unfortunately cannot at this moment
be terminated with preventDefault(). But it's nice to be notified when problems
occur!

Closes angular#5803
@caitp
Copy link
Contributor Author

caitp commented May 15, 2014

PTAL, I think that should be everything

@btford btford added this to the 1.3.0 milestone Jun 10, 2014
@btford btford self-assigned this Jun 10, 2014
@btford
Copy link
Contributor

btford commented Jun 10, 2014

Landed as e4419da.

@btford btford closed this Jun 10, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error event on ngInclude is needed.
6 participants