Skip to content

[build-utils] Add support for writing routes-manifest.json#7062

Merged
styfle merged 4 commits into
mainfrom
add-routes-manifest-conversion
Nov 24, 2021
Merged

[build-utils] Add support for writing routes-manifest.json#7062
styfle merged 4 commits into
mainfrom
add-routes-manifest-conversion

Conversation

@styfle

@styfle styfle commented Nov 24, 2021

Copy link
Copy Markdown
Member

@codecov

codecov Bot commented Nov 24, 2021

Copy link
Copy Markdown

Codecov Report

Merging #7062 (03696a8) into main (cc7b269) will not change coverage.
The diff coverage is n/a.

❗ Current head 03696a8 differs from pull request most recent head 799fbf6. Consider uploading reports for the commit 799fbf6 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7062   +/-   ##
=======================================
  Coverage   51.49%   51.49%           
=======================================
  Files         128      128           
  Lines        4979     4979           
  Branches     1172     1172           
=======================================
  Hits         2564     2564           
  Misses       2407     2407           
  Partials        8        8           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc7b269...799fbf6. Read the comment docs.

],
dynamicRoutes: [
{
page: '/api/project/[aid]/[bid]/index',

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@AndyBitz Does the page need to strip /index here?

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.

Good question. I think it doesn't matter, because /index and / should be equal

@AndyBitz AndyBitz Nov 24, 2021

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.

Ah, I think it'd be best to remove it, as I just checked what Next.js does: 

  "dynamicRoutes": [
    {
      "page": "/[nice]",
      "regex": "^/([^/]+?)(?:/)?$",
      "routeKeys": {
        "nice": "nice"
      },
      "namedRegex": "^/(?<nice>[^/]+?)(?:/)?$"
    }
  ]

 for a pages/[nice]/index.js

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.

Not relevant because Next.js will not create an index file for it, it'll make [nice] the file itself, so yea, we can leave it as it is.

pages404: true,
redirects: [],
rewrites: [
/* TODO: `handle: miss`

@styfle styfle Nov 24, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Currently, zero config routes have a handle: miss phase thats used to add a rewrite so that both the extension and the extensionless api file work (unless cleanUrls: true which will use a redirect instead).

However, the current routes-manifest.json doesn't support handle: miss routes, so we'll need to provide a new prop in the manifest if we want to keep the same behavior.

@styfle styfle merged commit 318bf35 into main Nov 24, 2021
@styfle styfle deleted the add-routes-manifest-conversion branch November 24, 2021 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants