X Tutup
Skip to content

fix(router): throw when component in route config is not defined#3569

Closed
btford wants to merge 1 commit intoangular:masterfrom
btford:fix-router-undefined-component
Closed

fix(router): throw when component in route config is not defined#3569
btford wants to merge 1 commit intoangular:masterfrom
btford:fix-router-undefined-component

Conversation

@btford
Copy link
Copy Markdown
Contributor

@btford btford commented Aug 10, 2015

Close #3265

@btford btford added comp: router action: review The PR is still awaiting reviews from at least one requested reviewer effort1: hours labels Aug 10, 2015
caitp
caitp reviewed Aug 10, 2015
View reviewed changes
modules/angular2/test/router/route_config_spec.ts Outdated
Copy link
Copy Markdown
Contributor

@caitp caitp Aug 10, 2015

Choose a reason for hiding this comment

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

does ts2dart understand undefined literals?

Copy link
Copy Markdown
Contributor Author

@btford btford Aug 10, 2015

Choose a reason for hiding this comment

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

these tests are only for JS-land

@IgorMinar IgorMinar assigned btford and unassigned IgorMinar Aug 10, 2015
@IgorMinar IgorMinar added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 10, 2015
@btford btford force-pushed the fix-router-undefined-component branch 2 times, most recently from e3688ca to f57a55e Compare August 10, 2015 21:30
@btford btford added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Aug 10, 2015
@btford btford assigned IgorMinar and unassigned btford Aug 10, 2015
@btford
Copy link
Copy Markdown
Contributor Author

btford commented Aug 10, 2015

@IgorMinar I've amended with your changes– mind taking another look?

btford
btford reviewed Aug 10, 2015
View reviewed changes
modules/angular2/test/router/route_config_spec.ts Outdated
Copy link
Copy Markdown
Contributor Author

@btford btford Aug 10, 2015

Choose a reason for hiding this comment

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

I'd like to add another assertion here with the wrong type (for ES5 users), but tsc won't even compile it. Is there a way to test ES5 code a user would write?

Copy link
Copy Markdown
Contributor

@caitp caitp Aug 10, 2015

Choose a reason for hiding this comment

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

it should compile it if you add <Type>(non-type), I think. well, maybe not :(

Copy link
Copy Markdown
Contributor

@caitp caitp Aug 10, 2015

Choose a reason for hiding this comment

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

http://stackoverflow.com/questions/14737518/is-there-a-way-to-ignore-type-incompatibility-in-typescript might explain how to do this --- hmmm, no, think this is still wrong.

Copy link
Copy Markdown
Contributor Author

@btford btford Aug 10, 2015

Choose a reason for hiding this comment

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

Nope: Neither type 'number' nor type 'Type' is assignable to the other. when I try to do something like <Type>4.

I'd be more comfortable adding this case in an un-processed route_config_impl.es5.js file, but I don't think this is easy to do with our current infrastructure.

Copy link
Copy Markdown
Contributor

@caitp caitp Aug 10, 2015

Choose a reason for hiding this comment

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

yeah. Maybe just leave a TODO() comment and leave it as-is, since it will only affect people writing non-typechecked code

Copy link
Copy Markdown
Contributor Author

@btford btford Aug 10, 2015

Choose a reason for hiding this comment

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

Aha! I can cast to any, then to Type: <Type>(<any>4)

@btford btford added this to the alpha-35 milestone Aug 10, 2015
@btford btford force-pushed the fix-router-undefined-component branch from f57a55e to d5a9048 Compare August 10, 2015 21:49
@btford
Copy link
Copy Markdown
Contributor Author

btford commented Aug 10, 2015

Hm, might have to move this logic outside the route definition classes into the recognizer or registry: https://travis-ci.org/angular/angular/jobs/74994261#L274

@IgorMinar
Copy link
Copy Markdown
Contributor

IgorMinar commented Aug 10, 2015

lgtm

@IgorMinar IgorMinar assigned btford and unassigned IgorMinar Aug 10, 2015
@IgorMinar IgorMinar added pr_state: LGTM and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 10, 2015
@IgorMinar IgorMinar added the action: merge The PR is ready for merge by the caretaker label Aug 10, 2015
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Aug 10, 2015

Merging PR #3569 on behalf of @IgorMinar to branch presubmit-IgorMinar-pr-3569.

@mary-poppins mary-poppins removed the action: merge The PR is ready for merge by the caretaker label Aug 10, 2015
@tbosch
Copy link
Copy Markdown
Contributor

tbosch commented Aug 10, 2015

@btford the build failed. Could you look into it?

@tbosch tbosch added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Aug 10, 2015
@btford
Copy link
Copy Markdown
Contributor Author

btford commented Aug 10, 2015

On it already– I'm pushing a fix now.

@btford btford force-pushed the fix-router-undefined-component branch 3 times, most recently from ae3a067 to df578ec Compare August 11, 2015 00:41
@btford btford force-pushed the fix-router-undefined-component branch from df578ec to 48edf25 Compare August 11, 2015 20:10
@btford
Copy link
Copy Markdown
Contributor Author

btford commented Aug 11, 2015

finally made all the compilers happy

computers are the worst

@btford btford closed this in 903a0f0 Aug 11, 2015
goderbauer pushed a commit to goderbauer/angular that referenced this pull request Aug 15, 2015
@btford @goderbauer
fix(router): throw when component in route config is not defined
7236de8 
Close angular#3265

Closes angular#3569
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Sep 6, 2019

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

Reviewers

No reviews

Assignees

@btford btford

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: yes effort1: hours

Projects

None yet

Milestone

alpha-35

Development

Successfully merging this pull request may close these issues.

RouteConfig: Non Existing Components Issue

6 participants

@btford @IgorMinar @mary-poppins @tbosch @caitp @googlebot
X Tutup