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

Improve error message for endpoint getStaticPaths with undefined value #6353

Merged
merged 70 commits into from
Mar 28, 2023

Conversation

JerryWu1234
Copy link
Contributor

@JerryWu1234 JerryWu1234 commented Feb 24, 2023

Update by bluwy:

Changes

Fixes #6095

Provide a better error message for #6095 as it's not possible to prerender the endpoint with a specific edge case shown in the changeset message. The error looks something like this:

 error   Could not render `/api/catch/[...slug]` with an `undefined` param as the generated path will collide during prerendering. Prevent passing `undefined` as `params` for the endpoint's `getStaticPaths()` function, or add an additional extension to the endpoint's filename.
  Hint:
    Rename `src/pages/api/catch/[...slug].ts` to `src/pages/api/catch/[...slug].json.ts`
  File:
    src/pages/api/catch/[...slug].ts
  Stacktrace:

Testing

Test added in Astro's integration tests

Docs

This will create a new Astro error for https://docs.astro.build/en/reference/error-reference/#astro-errors. Appreciate a review!

/cc @withastro/maintainers-docs for feedback!

End of update


Changes

related to #6095

Use the undefined to ignore an index in the function getStaticPaths when you want to access a website link, such as api/example/index .
I think there can't appear an undefined in the value of returned getStaticPaths.
For one, when an undefined in the array will break the bundle.
such as when config is

export default defineConfig({
  output: "server",
  adapter: node({mode:'standalone'}),
  build: {
    format: 'file'
  }
});

And there is an [...slug].astro in the directory pages/index

When [...slug].astro is

---
export const prerender = true;

export function getStaticPaths() {
  const slugs = ["one", undefined];

  return slugs.map((u) => ({ params: { slug: u } }));
}

const { slug } = Astro.params;
---

<h1>{slug || "index"}</h1>

Result:
The page/index/index.html will replace Index.html.

image

For another:
This situation also happens in theEndpoints.
It will fight to create the same file of trailing slash each other when the getStaticPaths return values of more than one, including an undefined.this also will cause a bug of #6095.
when it is

import type { APIRoute } from "astro";

const slugs = ["one", undefined];

export const prerender = true;

export const get: APIRoute = ({ params }) => {
  return {
    body: JSON.stringify({
      slug: params.slug || "index",
    }),
  };
};

export function getStaticPaths() {
  return slugs.map((u) => ({ params: { slug: u } }));
}

The path like api/example.
When the one will create a path as api/example/one.
When the undefined will create a path as api/example.
And will get an error:
error EISDIR: illegal operation on a directory, open '/Users/wuls/Desktop/issue/astro-bug-params-undefined/dist/client/api/example'.

Resolution:

My idea is to tell the users uses to replace the undefined with the index and check the value of returned getStaticPaths.
Automating to replace undefined with index or throwing an error message if there includes an undefined
or send a warn to users.

Such as:

export function getStaticPaths() {
  const slugs = ["one", "index"];

  return slugs.map((u) => ({ params: { slug: u } }));
}

Testing

Docs

https://docs.astro.build/en/reference/configuration-reference/#buildformat

@changeset-bot
Copy link

changeset-bot bot commented Feb 24, 2023

🦋 Changeset detected

Latest commit: 56d3473

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Feb 24, 2023
@JerryWu1234
Copy link
Contributor Author

JerryWu1234 commented Mar 1, 2023

There will have a compatible problem.
Such as users who previously used api/exmple to obtain data from the endpoints need to use api/exmple/indexto get data right now .

@JerryWu1234 JerryWu1234 marked this pull request as ready for review March 1, 2023 07:04
@bluwy
Copy link
Member

bluwy commented Mar 1, 2023

Ah alright, I see the issue now. Thanks for the write-up!

I don't think we can remove support for undefined yet because there are actual use case for it with slugs. Even though it has the conflict behaviour, e.g. [...slug].astro (with undefined) and index.astro, I think that is still correct because you're opting in to that. In which case we should log a warning that there's a conflict instead (probably future PR).

For endpoints, the problem seems to be that in static generation, we generate files without extensions, which conflict with folder names. I don't think we can add an extension here, that changes how it's served.

So I think the solution here is to log an error if undefined is used in endpoints getStaticPaths, where the endpoint file name is *.js or *.ts, and skip generating it statically. Because endpoints with *.json.js or *.json.ts will still work as that has an extension.

@bluwy bluwy changed the title Fix route Fix endpoint getStaticPaths with undefined value Mar 1, 2023
@JerryWu1234
Copy link
Contributor Author

Thanks for your reply.
I think this idea is better.
I will fix it late.

@JerryWu1234 JerryWu1234 requested a review from a team as a code owner March 2, 2023 03:08
@JerryWu1234
Copy link
Contributor Author

Ah alright, I see the issue now. Thanks for the write-up!

I don't think we can remove support for undefined yet because there are actual use case for it with slugs. Even though it has the conflict behaviour, e.g. [...slug].astro (with undefined) and index.astro, I think that is still correct because you're opting in to that. In which case we should log a warning that there's a conflict instead (probably future PR).

For endpoints, the problem seems to be that in static generation, we generate files without extensions, which conflict with folder names. I don't think we can add an extension here, that changes how it's served.

So I think the solution here is to log an error if undefined is used in endpoints getStaticPaths, where the endpoint file name is *.js or *.ts, and skip generating it statically. Because endpoints with *.json.js or *.json.ts will still work as that has an extension.

don't we need any tips if meet the *.js?
do we directly skip to generate ?

@JerryWu1234
Copy link
Contributor Author

Ah alright, I see the issue now. Thanks for the write-up!

I don't think we can remove support for undefined yet because there are actual use case for it with slugs. Even though it has the conflict behaviour, e.g. [...slug].astro (with undefined) and index.astro, I think that is still correct because you're opting in to that. In which case we should log a warning that there's a conflict instead (probably future PR).

For endpoints, the problem seems to be that in static generation, we generate files without extensions, which conflict with folder names. I don't think we can add an extension here, that changes how it's served.

So I think the solution here is to log an error if undefined is used in endpoints getStaticPaths, where the endpoint file name is *.js or *.ts, and skip generating it statically. Because endpoints with *.json.js or *.json.ts will still work as that has an extension.

Your second solution will break a lot of tests, this one cannot be skipped.

@JerryWu1234
Copy link
Contributor Author

@altano cc @bluwy

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Changes now look good to me! One more pass from docs and we should be good to go.

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Thanks so much for everyone's work on this! Signing off on a Docs review, so this PR is in all y'all's hands now! 🙌

Copy link
Member

@yanthomasdev yanthomasdev left a comment

Choose a reason for hiding this comment

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

Docs LGTM!

@bluwy bluwy merged commit 4bf87c6 into withastro:main Mar 28, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

undefined params within getStaticPaths cause Vercel build to fail
5 participants