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

feat(nxdev): redirects to send legacy tutorial links to new locations #12752

Merged
merged 5 commits into from Oct 24, 2022

Conversation

ZackDeRose
Copy link
Member

Current Behavior

Links to legacy docs (e.g. https://nx.dev/react-tutorial/01-create-application) lead to a 404 page.

Expected Behavior

User navigating to any old link get redirected to step 1 of the new tutorial (https://nx.dev/react-tutorial/1-code-generation).

Related Issue(s)

Fixes #

@vercel
Copy link

vercel bot commented Oct 21, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
nx-dev ⬜️ Ignored (Inspect) Oct 22, 2022 at 2:57AM (UTC)

@ZackDeRose
Copy link
Member Author

[still want to add tests for this; @bcabanes, can you let me know at what level we'd like to add these?]

@ZackDeRose
Copy link
Member Author

Hey @bcabanes, as I mentioned, I'm looking to add some testing to these to ensure the redirects work as directed. I was going to opt into a set of e2e tests to assert the redirect actually works (see below), but happy to move that to some unit test if you prefer that strategy (or these would be too expensive)

describe('Tutorial redirects', () => {
  const redirects: {
    givenUrl: string;
    expectedDestination: string;
  }[] = [
    {
      givenUrl: '/react-tutorial/01-create-application',
      expectedDestination: '/react-tutorial/1-code-generation',
    },
    {
      givenUrl: '/react-tutorial/02-add-e2e-test',
      expectedDestination: '/react-tutorial/1-code-generation',
    },
    // ...
  ];
  for (const redirect of redirects) {
    testRedirect(redirect);
  }
});

function testRedirect({
  givenUrl,
  expectedDestination,
}: {
  givenUrl: string;
  expectedDestination: string;
}) {
  it(`"${givenUrl}" to ${expectedDestination}`, () => {
    cy.visit(givenUrl);
    cy.url().should('include', expectedDestination);
  });
}

@bcabanes
Copy link
Member

bcabanes commented Oct 22, 2022

@ZackDeRose Instead of adding 24 new E2E tests that will slow down the whole CI run, we could change these tests to unit tests making sure the redirect rules are declared and their values are correct (without hitting the website).

There are multiple things here:

  1. moving the redirect rules into the /redirect-rules.config.js file
  2. creating unit tests making sure these redirect rules are present and their values are correct.
  3. removing the proposed regexps, we do not want a "catch-all" rule that would conflict with future paths.

@ZackDeRose ZackDeRose force-pushed the add-redirects-for-old-doc-links branch from f42528b to 686aba5 Compare October 22, 2022 02:56
@bcabanes bcabanes merged commit 8fcc2b4 into nrwl:master Oct 24, 2022
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants