Skip to content

fix(router): Fix relative link generation from empty path components#37446

Closed
atscott wants to merge 1 commit into
angular:masterfrom
atscott:emptyrelative
Closed

fix(router): Fix relative link generation from empty path components#37446
atscott wants to merge 1 commit into
angular:masterfrom
atscott:emptyrelative

Conversation

@atscott

@atscott atscott commented Jun 4, 2020

Copy link
Copy Markdown
Contributor

Partial resubmit of #26243
Fixes incorrect url tree generation for empty path components with children.
Adds a test to demonstrate the failure of createUrlTree for those routes.
Fixes #13011
Fixes #35687

@atscott atscott added type: bug/fix area: router target: patch This PR is targeted for the next patch release action: presubmit The PR is in need of a google3 presubmit router: URL parsing/generation labels Jun 4, 2020
@ngbot ngbot Bot modified the milestone: needsTriage Jun 4, 2020
@atscott atscott force-pushed the emptyrelative branch 2 times, most recently from 4809712 to 56c0023 Compare June 4, 2020 23:42
@atscott atscott marked this pull request as ready for review June 4, 2020 23:43
@atscott atscott force-pushed the emptyrelative branch 2 times, most recently from efee7fd to d7708b8 Compare June 5, 2020 18:13
@atscott

atscott commented Jun 5, 2020

Copy link
Copy Markdown
Contributor Author

There are two reproductions (https://stackblitz.com/edit/angular-lazy-route-relative-bug, https://stackblitz.com/edit/relative-link-eager-loading) in #35687 that aren't fixed by the change as it is now. However, both of those would work if the fix were changed to
return new Position(segmentGroup, false, 0);
instead of
return new Position(segmentGroup, segmentGroup.segments.length === 0, 0);

Adding cleanup, blocked, and presubmit label to indicate that I need to investigate if this fix can be extended to include #35687, adding a unit test to cover that situation, and running presubmits to ensure it doesn't break anything.

Edit: test for this case would be:

  it('should support pathless child of pathless root', () => {
    // i.e. routes = {path: '', loadChildren: () => import('child')...}
    // forChild: {path: '', component: Comp}
    const p = serializer.parse('');
    const empty = new UrlSegmentGroup([], {});
    p.root.children[PRIMARY_OUTLET] = empty;
    empty.parent = p.root;
    const t = create(empty, -1, p, ['lazy']);
    expect(serializer.serialize(t)).toEqual('/lazy');
  });

presubmit has some failures though

@atscott atscott added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews state: blocked labels Jun 5, 2020
@atscott

atscott commented Jun 6, 2020

Copy link
Copy Markdown
Contributor Author

global presubmit looks good

@atscott atscott force-pushed the emptyrelative branch 2 times, most recently from e2c141c to 21b4cd1 Compare June 8, 2020 15:37
@atscott

atscott commented Jun 8, 2020

Copy link
Copy Markdown
Contributor Author

global presubmit with additional change (processChildren = segmentGroup === tree.root instead of segmentGroup.segments.length === 0) to address #35687.

Some things to note for review:

  • All three added tests fail without the change
  • The generated links without the change are completely wrong (/a/(b) and //(lazy)) meaning this shouldn't be behavior anyone could have been relying on and this is purely a bug fix
  • Global presubmit shows no failures

@atscott atscott closed this Jun 8, 2020
Partial resubmit of angular#26243
Fixes incorrect url tree generation for empty path components with children.
Adds a test to demonstrate the failure of createUrlTree for those routes.
Fixes angular#13011
Fixes angular#35687
@atscott atscott reopened this Jun 8, 2020
@atscott atscott 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 action: presubmit The PR is in need of a google3 presubmit labels Jun 8, 2020

@AndrewKushnir AndrewKushnir 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.

LGTM 👍 Thanks for the fix @atscott!

@atscott atscott added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jun 8, 2020
@ngbot ngbot Bot added the action: merge The PR is ready for merge by the caretaker label Jun 8, 2020
@atscott

atscott commented Jun 8, 2020

Copy link
Copy Markdown
Contributor Author

merge assistance: I'm the router owner so pullapprove won't be happy without global approval

@atscott atscott closed this in 8d817da Jun 9, 2020
atscott added a commit that referenced this pull request Jun 9, 2020
…37446)

Partial resubmit of #26243
Fixes incorrect url tree generation for empty path components with children.
Adds a test to demonstrate the failure of createUrlTree for those routes.
Fixes #13011
Fixes #35687

PR Close #37446
ngwattcos pushed a commit to ngwattcos/angular that referenced this pull request Jun 25, 2020
…ngular#37446)

Partial resubmit of angular#26243
Fixes incorrect url tree generation for empty path components with children.
Adds a test to demonstrate the failure of createUrlTree for those routes.
Fixes angular#13011
Fixes angular#35687

PR Close angular#37446
@angular-automatic-lock-bot

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 Jul 10, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
…ngular#37446)

Partial resubmit of angular#26243
Fixes incorrect url tree generation for empty path components with children.
Adds a test to demonstrate the failure of createUrlTree for those routes.
Fixes angular#13011
Fixes angular#35687

PR Close angular#37446
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 action: review The PR is still awaiting reviews from at least one requested reviewer area: router cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note router: URL parsing/generation target: patch This PR is targeted for the next patch release type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

relative routing to a route with empty path causes weird urls and breaks the routing eventually RouterLink: incorrect relative link if defined in component having empty path

4 participants