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

fix(router): do not require the creation of empty-path routes when no… #12772

Closed
wants to merge 2 commits into from

Conversation

vsavkin
Copy link
Contributor

@vsavkin vsavkin commented Nov 8, 2016

Two Changes

Relax Matching

Previously having this configuration

{ path: 'parent', component: Parent, children: [
  { path: 'child', component: Child }
] }

the following url would not match '/parent'. You would have to create a bogus empty-path route, so the configuration looks like this:

{ path: 'parent', component: Parent, children: [
  { path: '', component: Blank },
  { path: 'child', component: Child }
] }

Now it is no longer required.

Custom Matching

We are adding support for doing custom url matching.

function endsWithHtml(url: UrlSegment[], group: UrlSegmentGroup, route: Route) {
  return url.length === 1 && url[0].path.endsWith(".htmlk") ? ({consumed: url}) : null;
}
{ matcher: endsWithHtml, component: HtmlCmp }

@vsavkin vsavkin added the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 8, 2016
@@ -655,13 +675,13 @@ describe('Integration', () => {
expect(cmp.activations.length).toEqual(1);
expect(cmp.activations[0] instanceof BlankCmp).toBe(true);

router.navigateByUrl('/simple');
router.navigateByUrl('/simple').catch(e => console.log(e));
Copy link
Contributor

Choose a reason for hiding this comment

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

debug ?

const parts = path.split('/');
const positionalParamSegments: {[k: string]: UrlSegment} = {};
const consumedSegments: UrlSegment[] = [];
const matcher = route.matcher ? route.matcher : defaultUrlMatcher;
Copy link
Contributor

Choose a reason for hiding this comment

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

route.matcher || defaultUrlMatcher

* @experimental
*/
export type UrlMatchResult = {
consumed: UrlSegment[], posParams?: {[name: string]: UrlSegment};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: mixed ,/;

* @experimental
*/
export type UrlMatchResult = {
consumed: UrlSegment[], posParams?: {[name: string]: UrlSegment};
Copy link
Contributor

Choose a reason for hiding this comment

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

consumedSegments to be consistent with previous file ?

*
* @description
*
* A custom URL matcher can be provided when a combination of `path` and `pathMatch` isn't
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: formatting

*
* ```
* function htmlFiles(url: UrlSegment[]) {
* return url.length === 1 && url[0].path.endsWith('.html') ? ({consuemd: url}) : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

typo consumed

const parts = path.split('/');
const posParameters: {[key: string]: any} = {};
const consumedSegments: UrlSegment[] = [];
const matcher = route.matcher ? route.matcher : defaultUrlMatcher;
Copy link
Contributor

Choose a reason for hiding this comment

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

||

@vicb vicb added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews pr_state: LGTM labels Nov 9, 2016
@vsavkin vsavkin force-pushed the router_loosen_up_matching branch from 028e889 to cf56501 Compare November 9, 2016 23:25
@vsavkin vsavkin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 9, 2016
@vicb vicb closed this in 7340735 Nov 10, 2016
@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 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants