Skip to content

fix(ui-view): change $viewContentLoading to pair with $viewContentLoaded #2003

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

Merged
merged 1 commit into from
Aug 11, 2015

Conversation

metamatt
Copy link

The old $viewContentLoaded event wasn't needed for the one internal
client (ui-view) because ui-view always ignores that event and acts on
a $stateChangeSuccess event that follows right behind anyway.

And it wasn't as useful to external clients as it could be because
it wasn't delivered on every view update -- it was delivered only
on state transitions that activate a state defining a view, and
didn't deal with inheritance.

Also, neither $viewContentLoading nor $viewContentLoaded events
contained the name of the view loading or loaded.

This change makes $viewContentLoading and $viewContentLoaded be
emitted always in pairs, before/after updateView does the work of
actually loading the view, and both events include the name of the
view being loaded.

This is a breaking change for users of the old $viewContentLoading
event that relied on it being broadcast from the root scope, the
precise time it was broadcast, the fact that it wasn't always sent
when a view is loaded even if $viewContentLoaded will be sent, or the
"options" parameter that was emitted with it. If people rely on any of
that behavior and we can't break it, then we can restore the old
behavior but I think there needs to be some other event which is
reliably paired with $viewContentLoaded and contains the view name,
and I can't think of a better name than $viewContentLoading for that
event.

Closes #685.

The old $viewContentLoaded event wasn't needed for the one internal
client (ui-view) because ui-view always ignores that event and acts on
a $stateChangeSuccess event that follows right behind anyway.

And it wasn't as useful to external clients as it could be because
it wasn't delivered on every view update -- it was delivered only
on state transitions that activate a state defining a view, and
didn't deal with inheritance.

Also, neither $viewContentLoading nor $viewContentLoaded events
contained the name of the view loading or loaded.

This change makes $viewContentLoading and $viewContentLoaded be
emitted always in pairs, before/after updateView does the work of
actually loading the view, and both events include the name of the
view being loaded.

This is a breaking change for users of the old $viewContentLoading
event that relied on it being broadcast from the root scope, the
precise time it was broadcast, the fact that it wasn't always sent
when a view is loaded even if $viewContentLoaded will be sent, or the
"options" parameter that was emitted with it. If people rely on any of
that behavior and we can't break it, then we can restore the old
behavior but I think there needs to be some other event which is
reliably paired with $viewContentLoaded and contains the view name,
and I can't think of a better name than $viewContentLoading for that
event.

Closes angular-ui#685.
@kevinob11
Copy link

I want this.

@christopherthielen
Copy link
Contributor

The old $viewContentLoaded event wasn't needed for the one internal client (ui-view) because ui-view always ignores that event and acts on a $stateChangeSuccess event that follows right behind anyway.

@metamatt thank you, I think your analysis is correct. I think this change would also address some issues with notify: false, as some other users mentioned.

This is a breaking change for users of the old $viewContentLoading event that relied on it being broadcast from the root scope, the precise time it was broadcast, the fact that it wasn't always sent
when a view is loaded even if $viewContentLoaded will be sent, or the "options" parameter that was emitted with it.

meh. I can't think of a scenario where you'd care about those things, except perhaps the options or $rootScope. I'm sure the one guy who relies on this will chime in to let us know eventually.

@nateabele if we accept this PR, let's bump our next release to 0.3.0 just to keep the semver folks off our backs. Also, we'd need to merge the relevant bits into feature-1.0 branch.

@christopherthielen
Copy link
Contributor

@kevinob11 @metamatt what do you use $viewContentLoading/Loaded for?

@kevinob11
Copy link

I don't use it at all. The issue I was running into was views reloading twice after a notify:false view change.

@metamatt
Copy link
Author

metamatt commented Aug 4, 2015

My use case:

my test harness wants to know when views are instantiating so that if a controller throws an exception, it knows which view to blame. So I really want a reliable "loading" signal.

(this and some additional thoughts are in #685 (comment))

@metamatt
Copy link
Author

metamatt commented Aug 4, 2015

And re

meh. I can't think of a scenario where you'd care about those things

I 100% agree. I was just being pedantic about describing the change in behavior to give anyone who would care a chance to notice and speak up. I would expect that anyone who is using these events would consider this change an improvement.

nateabele added a commit that referenced this pull request Aug 11, 2015
fix(ui-view): change $viewContentLoading to pair with $viewContentLoaded
@nateabele nateabele merged commit 997deb7 into angular-ui:master Aug 11, 2015
@nateabele
Copy link
Contributor

@christopherthielen Just merged this. I don't remember what my objection was before, but it makes sense to me now.

@btm1
Copy link

btm1 commented Aug 13, 2015

when will the next release happen? I want this 👍

@rcollette
Copy link

+1 Need a release. The lack of consistent event pairs is a problem for me.

@chence
Copy link

chence commented Oct 28, 2015

+1 need a release

@genu
Copy link

genu commented Nov 4, 2015

+1 for release

@abarnwell
Copy link

+1 for release 👍

@JohnBernardsson
Copy link

+1 for release! 😃

@rebing
Copy link

rebing commented Nov 18, 2015

+1 for release

@tchiloh
Copy link

tchiloh commented Nov 23, 2015

when will the next release happen?

@Xaiid
Copy link

Xaiid commented Dec 10, 2015

+1 release please :(

@skomma
Copy link

skomma commented Dec 10, 2015

+1 for release!

@kouki-o
Copy link

kouki-o commented Dec 10, 2015

+1 please release.

@davidouanounou
Copy link

+1 for release !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

event $viewContentLoading doesn't seem to work