Skip to content

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

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

Closed
wants to merge 1 commit into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Jun 4, 2020

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
Copy link
Contributor Author

atscott commented Jun 5, 2020

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
Copy link
Contributor Author

atscott commented Jun 6, 2020

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
Copy link
Contributor Author

atscott commented Jun 8, 2020

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
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

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
Copy link
Contributor Author

atscott commented Jun 8, 2020

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

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
4 participants