Skip to content

fix(auth): add scheduler to schedule onAuth events through Angular zone - #368

Merged
jeffbcross merged 1 commit into
angular:masterfrom
jeffbcross:zoneschedule
Jul 28, 2016
Merged

jeffbcross merged 1 commit into
angular:masterfrom
jeffbcross:zoneschedule

Conversation

@jeffbcross

Copy link
Copy Markdown
Contributor

Closes #354

@jeffbcross jeffbcross added type: bug Defect: expected behavior doesn't happen. comp: auth Authentication (src/auth). labels Jul 19, 2016
@jeffbcross jeffbcross added this to the Beta.3 milestone Jul 19, 2016
Comment thread src/auth/auth.spec.ts Outdated
authCb = null;
backend = new FirebaseSdkAuthBackend(app);
})();
var mockNgZone = Zone.current.fork({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment on why you need to wrap the code in a zone. What exactly needs to be wrapped ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code actually doesn't need to be run in a zone. I should fork the zone in the spec that checks the zone to make sure the subscription callback isn't in the <root> zone.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean an inline comment :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know; I'm saying this code is not actually necessary.

@vicb

vicb commented Jul 19, 2016

Copy link
Copy Markdown
Contributor

As discussed, please add a TODO comment to the code to fix the this properly. Is anyone working on a solution ?

@jeffbcross

Copy link
Copy Markdown
Contributor Author

@vicb I'm working on addressing the bigger issue with Zone+Rx outside of this. But for now I changed the PR to wrap the subscriber functions at subscribe time.

Comment thread src/auth/firebase_sdk_auth_backend.ts Outdated
}
}

function wrapSubscriberMethods<T>(obs: Observer<T>): Observer<T> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment in the code

@vicb

vicb commented Jul 20, 2016

Copy link
Copy Markdown
Contributor

Please add comments to the code for maintainabilty.

LGTM once the comments are adressed

@jeffbcross

Copy link
Copy Markdown
Contributor Author

I'm going to make zone a required argument of ZoneScheduler, then will merge once green.

@jeffbcross
jeffbcross merged commit 3615318 into angular:master Jul 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes comp: auth Authentication (src/auth). pr_action: review type: bug Defect: expected behavior doesn't happen.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants