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(gatsby): don't force leading slash for external paths in routes manifest #38639

Conversation

jenae-janzen
Copy link
Contributor

@jenae-janzen jenae-janzen commented Oct 16, 2023

Fixes: https://linear.app/netlify/issue/FRA-20/external-redirects-have-a-forced-slash-added

If route.path begins with http(s)://, we shouldn't add a leading slash to it.

Description

Documentation

Tests

Related Issues

Fixes FRA-20

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 16, 2023
@jenae-janzen
Copy link
Contributor Author

🤔 Not sure why the tests are failing, I don't see that they're related to this change:

Screenshot 2023-10-16 at 3 11 33 PM

@pieh pieh added topic: adapters Related to Gatsby Adapters and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Oct 17, 2023
@MarcL
Copy link

MarcL commented Oct 17, 2023

Awesome! Can we add a unit test for this code @jenae-janzen?

The other unit tests are here:
https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/adapter/__tests__/manager.ts#L47

cc @pieh for any guidance if needed.

@pieh
Copy link
Contributor

pieh commented Oct 17, 2023

🤔 Not sure why the tests are failing, I don't see that they're related to this change:

Some tests are quite flaky :( - the failing test suites are not even using/testing any of code changes (I did just restart them)

cc @pieh for any guidance if needed.

const redirects: IGatsbyState["redirects"] = [{
fromPath: '/old-url',
isPermanent: true,
ignoreCase: true,
redirectInBrowser: false,
toPath: '/new-url'
}]
this would need some redirects added that would have external fromPath and then assertions/ tests done in https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/adapter/__tests__/manager.ts#L47

@@ -276,7 +276,8 @@ function getRoutesManifest(): RoutesManifest {

// TODO: This could be a "addSortedRoute" function that would add route to the list in sorted order. TBD if necessary performance-wise
function addRoute(route: Route): void {
if (!route.path.startsWith(`/`)) {
const externalPathsRegex = new RegExp(`^https?://`, `i`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any performance concerns using a regex here, i.e. if a site has a lot of redirects?

Copy link
Contributor

@pieh pieh Oct 18, 2023

Choose a reason for hiding this comment

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

doing route.path.startsWith('http://') || route.path.startsWith('https://') is faster than using that exact regex (tho doesn't cover case insensitivity) - so I'll adjust to use this.

"RegExp x 1,614,517 ops/sec ±0.62% (96 runs sampled)"
"startWith x 5,467,939 ops/sec ±0.17% (96 runs sampled)"
"Fastest is startWith"

Note here that it seems like initiating new regexp on each addRoute call is siginificant portion of overhead it seems - after moving regexp to be inited once earlier we get

"RegExp x 2,410,360 ops/sec ±0.99% (95 runs sampled)"
"startWith x 5,037,209 ops/sec ±3.31% (93 runs sampled)"
"Fastest is startWith"

so 2 startWith calls is still faster then regexp test (the startWith benchmark case was using 2 calls, not 1, so this will be ~twice as fast)

I'll adjust to use startWith and get PR merged

@pieh pieh changed the title Don't force leading slash for external redirects fix(gatsby): don't force leading slash for external redirects in routes manifest Oct 18, 2023
@pieh pieh changed the title fix(gatsby): don't force leading slash for external redirects in routes manifest fix(gatsby): don't force leading slash for external paths in routes manifest Oct 18, 2023
@pieh pieh added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Oct 18, 2023
@gatsbybot gatsbybot merged commit 5dbcf9e into gatsbyjs:master Oct 18, 2023
1 check passed
pieh pushed a commit that referenced this pull request Oct 19, 2023
…anifest (#38639)

* don't force leading slash for external redirects

* add test

* perf: regexp -> double String.startWith checks

---------

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit 5dbcf9e)
gatsbybot added a commit that referenced this pull request Oct 19, 2023
…anifest (#38639) (#38646)

* don't force leading slash for external redirects

* add test

* perf: regexp -> double String.startWith checks

---------

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit 5dbcf9e)

Co-authored-by: Jenae Janzen <101715009+jenae-janzen@users.noreply.github.com>
@pieh
Copy link
Contributor

pieh commented Oct 20, 2023

Released in gatsby@5.12.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: adapters Related to Gatsby Adapters
Projects
Status: Published
Development

Successfully merging this pull request may close these issues.

4 participants