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

4.0.0-rc.2 - es5 - CanDeactivate with EventEmitter #14926

Closed
uparlange opened this issue Mar 4, 2017 · 20 comments · Fixed by #15171
Closed

4.0.0-rc.2 - es5 - CanDeactivate with EventEmitter #14926

uparlange opened this issue Mar 4, 2017 · 20 comments · Fixed by #15171
Assignees
Labels
area: build & ci Related the build and CI infrastructure of the project area: packaging Issues related to Angular's creation of npm packages regression Indicates than the issue relates to something that worked in a previous version type: bug/fix

Comments

@uparlange
Copy link

uparlange commented Mar 4, 2017

I'm submitting a ... (check one with "x")

[ * ] bug report => search github for a similar issue or PR before submitting
[ ] feature request
[ ] support request => Please do not submit support request here, instead see

Current behavior
CanDeactivate does not work with EventEmitter !

Expected behavior
CanDeactivate should work with true/false and with EventEmitter

Minimal reproduction of the problem with instructions
https://embed.plnkr.co/tPtY1a31CnFqhNbzVViU/
--> Unable to return to Home when List Displayed

What is the motivation / use case for changing the behavior?
CanDeactivate should work with true/false and with EventEmitter

Please tell us about your environment:
Windows 7 / VisualCode / NPM / Express

  • Angular version: 2.0.X
    4.0.0-rc.2

  • Browser: [all | Chrome XX | Firefox XX | IE XX | Safari XX | Mobile Chrome XX | Android X.X Web Browser | iOS XX Safari | iOS XX UIWebView | iOS XX WKWebView ]
    All

  • Language: [all | TypeScript X.X | ES6/7 | ES5]
    ES5

  • Node (for AoT issues): node --version =
    Not concerned

@uparlange uparlange changed the title 4.0.0-rc.2 es5 - CanDeactivate with EventEmitter 4.0.0-rc.2 - es5 - CanDeactivate with EventEmitter Mar 4, 2017
@DzmitryShylovich
Copy link
Contributor

did you try .take(1)?

@uparlange
Copy link
Author

Can you be more specific ?

@DzmitryShylovich
Copy link
Contributor

You need to complete the stream.

@uparlange
Copy link
Author

Ok... Did you try with my plnkr sample... It does not seem to change anything... except having errors !

@DzmitryShylovich
Copy link
Contributor

DzmitryShylovich commented Mar 4, 2017

I'm not familiar with angular es5 but ts version works fine http://plnkr.co/edit/FGGSd80pDDp5HTCGGC8y?p=preview so it's something wrong with your config and not an angular issue

Plus your example doesn't even work with the most trivial Observable.of(true)

@uparlange
Copy link
Author

uparlange commented Mar 4, 2017

Your sample is not the same as mine as i'm lazy loading the components.
My CanDeactivate Guard is defined on child module and not in main module.

@DzmitryShylovich
Copy link
Contributor

@uparlange
Copy link
Author

uparlange commented Mar 4, 2017

Humm ok.
What i don't understand is that it was working with 4.0.0-rc.1 and workaround described in #14603.
It is not working anymore with 4.0.0-rc.2.
I've updated the plunker to show you : https://embed.plnkr.co/tPtY1a31CnFqhNbzVViU/

@IgorMinar IgorMinar added area: core Issues related to the framework runtime type: bug/fix labels Mar 13, 2017
@IgorMinar IgorMinar modified the milestones: 4.0.0, 4.0.0-blockers, 4.0.0-candidates Mar 13, 2017
@tbosch
Copy link
Contributor

tbosch commented Mar 14, 2017

@uparlange did this work in 2.x?

@tbosch
Copy link
Contributor

tbosch commented Mar 14, 2017

@uparlange Your latest punker seems to work, i.e. I can navigate from Home to List and back...

@DzmitryShylovich
Copy link
Contributor

@tbosch the latest plunkr uses rc1.

What i don't understand is that it was working with 4.0.0-rc.1. It is not working anymore with 4.0.0-rc.2.

@DzmitryShylovich
Copy link
Contributor

it does work with ts version so it's something wrong with es5 distribution. I'm not very familiar with it

@tbosch tbosch added area: router and removed area: core Issues related to the framework runtime labels Mar 14, 2017
@tbosch
Copy link
Contributor

tbosch commented Mar 14, 2017

Ok, I can reproduce now if I change the latest plunker manually to use v2 (i.e. change index.html to load v2...)

@tbosch tbosch modified the milestones: 4.0.0-blockers, 4.0.0-blocker-candidates Mar 14, 2017
@vicb
Copy link
Contributor

vicb commented Mar 15, 2017

Nothing has changed in the router in-between rc1 and rc2

Night be related to #14603 / #14820 (the latter was part of rc2), @jasonaden could you please chime in ?

Note: seems to work with rc1 and no more in rc2 and rc3.

@jasonaden
Copy link
Contributor

There was a change after rc.1 (move from babel back to rollup to generate UMDs) which made the workaround (adding to global Rx) stop working. We are looking at how to get a fix in for ReactiveX/rxjs#2415 or a usable workaround that should fix this issue for the next RC.

@jasonaden jasonaden self-assigned this Mar 15, 2017
@uparlange
Copy link
Author

Meanwhile and as a workaround we can change the eventemitter way by a Promise.

canDeactivate:function(component)
{
return new Promise((resolve) =>
{
resolve(true);
});
}

@tbosch tbosch added the area: build & ci Related the build and CI infrastructure of the project label Mar 15, 2017
@DzmitryShylovich
Copy link
Contributor

@jasonaden don't look like anybody cares in rxjs repo :)

As a workaround I suggest to change impl and just check if object has subscribe property (like it was in rc1). It's nice to use observable symbol, but if it causes issues I think we can live without it. I can send a PR

@jasonaden
Copy link
Contributor

@DzmitryShylovich Sounds good on the PR.

DzmitryShylovich added a commit to DzmitryShylovich/angular that referenced this issue Mar 15, 2017
@DzmitryShylovich
Copy link
Contributor

@jasonaden done #15171

DzmitryShylovich added a commit to DzmitryShylovich/angular that referenced this issue Mar 15, 2017
@chuckjaz chuckjaz added the regression Indicates than the issue relates to something that worked in a previous version label Mar 15, 2017
DzmitryShylovich added a commit to DzmitryShylovich/angular that referenced this issue Mar 15, 2017
@IgorMinar IgorMinar added the area: packaging Issues related to Angular's creation of npm packages label Mar 15, 2017
DzmitryShylovich added a commit to DzmitryShylovich/angular that referenced this issue Mar 15, 2017
chuckjaz pushed a commit that referenced this issue Mar 16, 2017
SamVerschueren pushed a commit to SamVerschueren/angular that referenced this issue Mar 18, 2017
asnowwolf pushed a commit to asnowwolf/angular that referenced this issue Aug 11, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this issue Aug 28, 2017
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: build & ci Related the build and CI infrastructure of the project area: packaging Issues related to Angular's creation of npm packages regression Indicates than the issue relates to something that worked in a previous version type: bug/fix
Projects
None yet
7 participants