Skip to content

fix(router): make locationSyncBootstrapListener public due to change … - #22896

Closed
jasonaden wants to merge 2 commits into
angular:masterfrom
jasonaden:fix_router_upgrade
Closed

fix(router): make locationSyncBootstrapListener public due to change …#22896
jasonaden wants to merge 2 commits into
angular:masterfrom
jasonaden:fix_router_upgrade

Conversation

@jasonaden

@jasonaden jasonaden commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

…in output after TS 2.7 update in #22669

See build diff on 2.7 here:

angular/router-builds@8788b39#diff-068e657ab436c99f423c5ad8d4187c1b

@jasonaden
jasonaden requested a review from IgorMinar March 20, 2018 20:58
IgorMinar
IgorMinar suggested changes Mar 20, 2018
View reviewed changes

@IgorMinar IgorMinar left a comment

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.

can't you type the useFactory prop as (ngUpgrade: UpgradeModule) => () => void by casting the initializer value?

we shouldn't export random private stuff just to make the compiler happy.

@mary-poppins

mary-poppins commented Mar 20, 2018

Copy link
Copy Markdown

You can preview de94b19 at https://pr22896-de94b19.ngbuilds.io/.

@jasonaden
jasonaden force-pushed the fix_router_upgrade branch from de94b19 to 37a5d18 Compare March 20, 2018 21:41
@jasonaden

jasonaden commented Mar 20, 2018

Copy link
Copy Markdown
Contributor Author

@IgorMinar Sounds good. Updated.

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Mar 20, 2018
@ngbot

ngbot Bot commented Mar 20, 2018

Copy link
Copy Markdown

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/circleci: lint" is failing
    pending status "ci/circleci: build" is pending
    pending status "google3" is pending
    pending status "continuous-integration/travis-ci/pr" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@IgorMinar

IgorMinar commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

@jasonaden please reformat

@mary-poppins

mary-poppins commented Mar 20, 2018

Copy link
Copy Markdown

You can preview 37a5d18 at https://pr22896-37a5d18.ngbuilds.io/.

@mary-poppins

mary-poppins commented Mar 20, 2018

Copy link
Copy Markdown

You can preview 86e68b1 at https://pr22896-86e68b1.ngbuilds.io/.

@IgorMinar IgorMinar closed this in 623d769 Mar 21, 2018
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 13, 2019

Copy link
Copy Markdown

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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@IgorMinar IgorMinar IgorMinar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jasonaden @mary-poppins @IgorMinar @googlebot