Skip to content

Bugfix in the legacy codebase #3027

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.

Sign up for GitHub

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

Jump to bottom
Closed
zeusdeux opened this issue Sep 21, 2016 · 8 comments
Closed

Bugfix in the legacy codebase #3027

zeusdeux opened this issue Sep 21, 2016 · 8 comments
Milestone
0.3.2

Comments

@zeusdeux
Copy link

Would y'all be ok with a PR that is based on legacy branch?

The PR will address an issue with ui-sref where it incorrectly discards the promise returned by $state.go and doesn't handle promise rejection.

It will also fix the erroneous behaviour outlined in #2123 if the state transition was triggered by ui-sref directive. Also, the change will be isolated to stateDirective.js so it shouldn't impede current dev efforts in any way.

Thanks!
@christopherthielen
Copy link
Contributor

yep! Is the issue fixed in 1.0 already? If not, I'd like a PR for both master and legacy

@zeusdeux
Copy link
Author

From the looks of it it isn't fixed on master yet.

I am unsure that my solution would apply to 1.0 though since it involves triggering a state change event. More specifically $stateChangeCancel when a transition is superseded.

@christopherthielen
Copy link
Contributor

More specifically $stateChangeCancel when a transition is superseded.

Interesting... although it doesn't seem like that should be handled by the ui-sref directive. It seems like the transitionTo should be doing that.

Can you explain fully what the issue you're solving is?

@zeusdeux
Copy link
Author

zeusdeux commented Sep 21, 2016 •
edited
Loading

I would absolutely love it if transitionTo broadcasted $stateChangeCancel on $rootScope when it rejects with TransitionSuperseded. The reasons why I chose not to do it in transitionTo are:

  1. transitionTo method is quite involved and it is quite easy to introduce a bug in it
  2. it would change $stateChangeCancel trigger semantics across the board
  3. I wanted to keep the approach more silo-ed so that it didn't impact part of the codebase I don't know

Now, the issue I am trying to solve is quite straightforward. I am in some state A and I start a transition to state B. Before the transition to B completes, I transition back to A. In this scenario, in the legacy world, I am not notified of anything and the locals for state B are resolved anyway. None of the $stateXYZ nor $viewContentXYZ events fire. As a consumer of the api, if I want to do something as simple as show a loader between transitions, the current behaviour would lead to the loader being stuck since no event is fired to notify me that the transition was cancelled.

In a nutshell, if $stateChangeStart fired, then logic dictates that an event that acknowledges success/error/failure should be fired too. That is not the case in the legacy world where there is silence after $stateChangeStart is triggered for state B.

zeusdeux added a commit to zeusdeux/ui-router that referenced this issue Sep 22, 2016
@zeusdeux
fix(ui-sref): handle state transition promise rejection in ui-sref di…
b6a2186 
…rective

In the current world, ui-sref does nothing when the promise returned by
$state.go is rejected. This PR gives ui-sref directive the ability to
emit $stateChangeCancel when the transition promise rejects.
The reason why I have chosen to implement this in the ui-sref directive
and not in the definition for $state.go or $state.transitionTo are
outlined in the issue below.

angular-ui#3027.
@zeusdeux zeusdeux mentioned this issue Sep 22, 2016
Closed
zeusdeux added a commit to zeusdeux/ui-router that referenced this issue Sep 22, 2016
@zeusdeux
fix(ui-sref): handle state transition promise rejection in ui-sref di…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
9658739 
…rective

In the current world, ui-sref does nothing when the promise returned by
$state.go is rejected. This PR gives ui-sref directive the ability to
emit $stateChangeCancel when the transition promise rejects.
The reason why I have chosen to implement this in the ui-sref directive
and not in the definition for $state.go or $state.transitionTo are
outlined in the issue below.

angular-ui#3027.
zeusdeux added a commit to zeusdeux/ui-router that referenced this issue Sep 22, 2016
@zeusdeux
fix(ui-sref): handle state transition promise rejection

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
700a207 
In the current world, ui-sref does nothing when the promise returned by
$state.go is rejected. This PR gives ui-sref directive the ability to
emit $stateChangeCancel when the transition promise rejects.
The reason why I have chosen to implement this in the ui-sref directive
and not in the definition for $state.go or $state.transitionTo are
outlined in the issue below.

angular-ui#3027.
zeusdeux added a commit to zeusdeux/ui-router that referenced this issue Sep 22, 2016
@zeusdeux
fix(ui-sref): handle state transition promise rejection

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
3b6c735 
In the current world, ui-sref does nothing when the promise returned by
$state.go is rejected. This PR gives ui-sref directive the ability to
emit $stateChangeCancel when the transition promise rejects.
The reason why I have chosen to implement this in the ui-sref directive
and not in the definition for $state.go or $state.transitionTo are
outlined in the issue below.

angular-ui#3027.
@zeusdeux
Copy link
Author

@christopherthielen Any thoughts?

Thanks!

@christopherthielen christopherthielen added this to the 0.3.2 milestone Sep 23, 2016
@christopherthielen
Copy link
Contributor

@zeusdeux I commented on your PR that I'd like to implement this correctly in transitionTo.
I'll put it on a ilestone for a legacy 0.3.2 release. I'm happy to help you update your PR, or you can wait until I get around to doing it myself.

zeusdeux added a commit to zeusdeux/ui-router that referenced this issue Sep 26, 2016
@zeusdeux
fix($state.transitionTo): trigger $stateChangeCancel appropriately
ca7dba4 
Current behaviour:

We are in state A. We start a transition to state B. `$stateChangeStart`
is triggered. Before `$stateChangeSuccess` is triggered for state B, we
start a transition back to state A. No state events are triggered and we
are left hanging without any notification.

Updated behaviour:

When the state change to B is superseded by the state change back to A,
`$stateChangeCancel` is broadcasted on `$rootScope` for the transition from
B -> A. This behaviour makes sure that for every `$stateChangeStart` there
is a corresponding `$stateChange<Success|Error|Cancel>` thus completing the
lifecycle.

Fixes issue angular-ui#3027
@zeusdeux zeusdeux mentioned this issue Sep 26, 2016
Merged
@zeusdeux
Copy link
Author

@christopherthielen I went ahead and PR-ed the implementation in transitionTo (#3039). I would love for you to take a look :)

Thanks!

zeusdeux added a commit to zeusdeux/ui-router that referenced this issue Sep 26, 2016
@zeusdeux
fix($state.transitionTo): trigger $stateChangeCancel appropriately
71b5b61 
Current behaviour:

We are in state A. We start a transition to state B. `$stateChangeStart`
is triggered. Before `$stateChangeSuccess` is triggered for state B, we
start a transition back to state A. No state events are triggered and we
are left hanging without any notification.

Updated behaviour:

When the state change to B is superseded by the state change back to A,
`$stateChangeCancel` is broadcasted on `$rootScope` for the transition from
B -> A. This behaviour makes sure that for every `$stateChangeStart` there
is a corresponding `$stateChange<Success|Error|Cancel>` thus completing the
lifecycle.

Fixes issue angular-ui#3027
christopherthielen pushed a commit that referenced this issue Nov 3, 2016
@zeusdeux @christopherthielen
fix($state.transitionTo): trigger $stateChangeCancel appropriately (#…
ca7c366 
…3039)

Current behaviour:

We are in state A. We start a transition to state B. `$stateChangeStart`
is triggered. Before `$stateChangeSuccess` is triggered for state B, we
start a transition back to state A. No state events are triggered and we
are left hanging without any notification.

Updated behaviour:

When the state change to B is superseded by the state change back to A,
`$stateChangeCancel` is broadcasted on `$rootScope` for the transition from
B -> A. This behaviour makes sure that for every `$stateChangeStart` there
is a corresponding `$stateChange<Success|Error|Cancel>` thus completing the
lifecycle.

Fixes issue #3027
@christopherthielen
Copy link
Contributor

closed by ca7c366

@WhileTruu WhileTruu mentioned this issue May 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.3.2
Development

No branches or pull requests
2 participants
@zeusdeux @christopherthielen