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

When creating a page with getStaticPaths which return a number param, later Astro.params returns a string #4309

Closed
1 task
marceloverdijk opened this issue Aug 13, 2022 · 17 comments

Comments

@marceloverdijk
Copy link

What version of astro are you using?

1.0.5

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

npm

What operating system are you using?

Mac

Describe the Bug

When creating a page with getStaticPaths which return a number param, later Astro.params returns a string.

---
import Layout from '../../layouts/Layout.astro';

export function getStaticPaths() {
  return [
    { params: { year: 1990 } }, // year is a number
    { params: { year: 1991 } },
    { params: { year: 1992 } },
  ]
}

const { year } = Astro.params
---

<Layout title="Season">
  {JSON.stringify(year)}
</Layout>

when accessing /seasons/1990 this prints:

"1990"

although expected:

1990

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-ydxdyc?file=src/pages/seasons/[year].astro

Participation

  • I am willing to submit a pull request for this issue.
@marceloverdijk
Copy link
Author

Also trying with explicit typing the year to a number using a Params interface, the year value is still a string.

---
import Layout from '../../layouts/Layout.astro';

export function getStaticPaths() {
  return [
    { params: { year: 1990 } }, // year is a number
    { params: { year: 1991 } },
    { params: { year: 1992 } },
  ]
}

interface Params extends Record<string, string | number | undefined> {
  year: number
}

const { year } = Astro.params as Params
---

<Layout title="Season">
  {JSON.stringify(year)}
</Layout>

@matthewp
Copy link
Contributor

matthewp commented Aug 15, 2022

Astro.params get pulled from the URL. They will always be strings. You can return props from getStaticPaths as well which will be on Astro.props and those can be numbers or anything else.

@marceloverdijk
Copy link
Author

marceloverdijk commented Aug 15, 2022

Hi @matthewp , thx for giving feedback.

I'm wondering if Astro.params are alway strings why is the type of Astro.params then:

export declare type Params = Record<string, string | number | undefined>;

Also note this PR , merged earlier this year:
#3087 - Fix/numeric path params
which included the number type to Params...

- export type Params = Record<string, string | undefined>;
+ export type Params = Record<string, string | number | undefined>;

Although I now also notice this:

❯ npm run dev

[email protected] dev
astro dev

🚀 astro v1.0.5 started in 54ms

┃ Local http://localhost:3000/
┃ Network use --host to expose

08:03:12 PM [getStaticPaths] invalid path param: year. A string value was expected, but got 1990.
08:03:12 PM [getStaticPaths] invalid path param: year. A string value was expected, but got 1991.
08:03:12 PM [getStaticPaths] invalid path param: year. A string value was expected, but got 1992.

I get this with:

export function getStaticPaths() {
  return [
    { params: { year: 1990 } }, // year is a number
    { params: { year: 1991 } },
    { params: { year: 1992 } },
  ]
}

which is basically similar as https://github.com/withastro/astro/blob/main/packages/astro/test/fixtures/astro-get-static-paths/src/pages/blog/%5Byear%5D/%5Bslug%5D.astro
but gives that warning/error message in the console..
so it's a little bit unclear to be honest.

I took your suggestion using Astro.props:

export function getStaticPaths() {
  return [
    { params: { year: "1990" }, props: { year: 1990 } },
    { params: { year: "1991" }, props: { year: 1991 } },
    { params: { year: "1992" }, props: { year: 1992 } },
  ]
}

export interface Props {
  year: number
}

const { year } = Astro.props as Props

This works. No warning/errors about "A string value was expected" and the year is a number.

But it feels a bit clumsy ...

@matthewp
Copy link
Contributor

Oh, let me reopen, it looks like the history is more complex than I thought.

@matthewp matthewp reopened this Aug 15, 2022
@marceloverdijk
Copy link
Author

Thx Matthew,

image

The docs also state:

so only strings and numbers are supported as values


So just to summarise, I see 2 issues:

(1)

Using a number in getStaticPaths gives an error/warning in the console.

export function getStaticPaths() {
  return [
    { params: { year: 1990 } }, // year is a number
    { params: { year: 1991 } },
    { params: { year: 1992 } },
  ]
}
> astro dev

  🚀  astro  v1.0.5 started in 39ms

  ┃ Local    http://localhost:3000/
  ┃ Network  use --host to expose

08:39:10 PM [getStaticPaths] invalid path param: year. A string value was expected, but got `1990`.
08:39:10 PM [getStaticPaths] invalid path param: year. A string value was expected, but got `1991`.
08:39:10 PM [getStaticPaths] invalid path param: year. A string value was expected, but got `1992`.

(2)

Using a number param in getStaticPaths, then when trying to access it later using Astro.params it gives a string (but number would be expected)

@marceloverdijk
Copy link
Author

Can it be confirmed if this is a bug or intended behavior?

And maybe label the issue accordingly?

@matthewp
Copy link
Contributor

Astro.params will never be anything other than strings because it comes from the URL. When the params are created it is not aware of the types that getStaticPaths returns. In SSR it will never know. So I think that part is a typing mistake.

The warning you're seeing I'm not sure about and will need to keep investigating. I would think that it should be ok to return a number but perhaps I am missing something.

@marceloverdijk
Copy link
Author

Yes for SSR I fully understand, not for SSG.

And I can also understand to take the string variant only then.

Maybe @natemoo-re @bholmesdev @tony-sull can chip in about that typing change in the merged PR #3087

@bholmesdev
Copy link
Contributor

@marceloverdijk Ah, was just asking myself this same question! It looks that PR allows numeric params when defining getStaticPaths, but does not add numeric param support for file-based routing. I know Svelte introduced type guards for file-based params (in their old routing setup at least), so we could definitely support this in Astro as well.

I agree this half-support for numeric params is confusing though; if I as a maintainer am unsure, I can only imagine how users feel 😅 @tony-sull any thoughts on this?

@tony-sull
Copy link
Contributor

I think the original motivation behind #3087 was to clean up a common pain point we were seeing in support threads at the time, there were a number of users trying to either use page numbers or dates in getStaticPaths() and getting bit by having to always .toString() the path params

The idea was to allow getStaticPaths() responses to include numbers since they'll just be stringified anyway, but since Astro.params is always parsed from a URL it will still return strings as it did before.

I think any change here to getStaticPaths or Astro.params would be breaking so we'd have to revisit that as part of a 2.0 release, could make for a really interesting RFC discussion to see what the general expectation for the API is. Before #3087 we had a more strict "strings in, strings out" API, maybe supporting numbers is more confusing than it's worth?

@tony-sull
Copy link
Contributor

08:39:10 PM [getStaticPaths] invalid path param: year. A string value was expected, but got `1990`.
08:39:10 PM [getStaticPaths] invalid path param: year. A string value was expected, but got `1991`.
08:39:10 PM [getStaticPaths] invalid path param: year. A string value was expected, but got `1992`.

Short term I think we should probably clean up this error message. I wouldn't have expect this to log since getStaticPaths is documented to support strings and numbers

@marceloverdijk
Copy link
Author

OK clear,

So with the current half-support I think I should use simply string's.

This is what I have now:

---
export async function getStaticPaths() {
  const seasons = await prisma.season.findMany({
    select: {
      year: true,
    },
  })
  return seasons.map((season) => {
    return {
      params: {
        year: season.year.toString(), // is number, but convert to string for getStaticPaths
      },
    }
  })
}

interface Params extends Record<string, string | number | undefined> { // is this the best way to define type safe params?
  year: string // define as string
}

const { year } = Astro.params as Params

const yearAsNumber = Number(year) 
---

{JSON.stringify(yearAsNumber)}

The output in the page is an example btw ;-)

ps: the way I defined the type sage Params, is that the most straightforward way to do it? By extending Record<string, string | number | undefined>

@bholmesdev
Copy link
Contributor

bholmesdev commented Sep 7, 2022

@marceloverdijk That looks good to me! We'll clean up the logs so you don't have to use toString() though. To clarify what I meant by "half support:"

  • if you pass a number to params.year in your getStaticPaths, it should still be a number when destructuring Astro.params. If not, that's a bug.
  • if you created an SSR dynamic route like [year].astro, year will always be a string (even if it could be parsed to a number). This is where string -> number conversion would be manual.
    ☝️ That nuance feels a bit confusing to me. But for 1.0, I'd expect that behavior to stick around.

@marceloverdijk
Copy link
Author

marceloverdijk commented Sep 7, 2022

@bholmesdev

if you pass a number to params.year in your getStaticPaths, it should still be a number when destructuring Astro.params. If not, that's a bug.

Indeed the toString() is not needed.

This works as well:

---
export async function getStaticPaths() {
  const seasons = await prisma.season.findMany({
    select: {
      year: true,
    },
  })
  return seasons.map((season) => {
    return {
      params: {
        year: season.year, // is number
      },
    }
  })
}

interface Params extends Record<string, string | number | undefined> {
  year: string
}

const yearAsNumber = Number(year) 
---

{JSON.stringify(yearAsNumber)}

The clean-up of the logs would be nice indeed

and I think I can change:

interface Params extends Record<string, string | number | undefined> {
  year: string
}

to

interface Params extends Record<string, string | undefined> {
  year: string
}

as that number doesn't make sense there anymore.

@rishi-raj-jain
Copy link
Contributor

  • I believe the docs have been updated to reflect this change.
  • I believe that the number to string conversion is also accounted for.
  • @matthewp this seems ready to be closed

@matthewp
Copy link
Contributor

matthewp commented Oct 6, 2022

Awesome, thank you @rishi-raj-jain !

@matthewp matthewp closed this as completed Oct 6, 2022
@georgeiliadis91
Copy link

georgeiliadis91 commented Oct 28, 2022

Hello, I am actually seeing a very similar thing.

I am trying to create a SSR slug route, and when trying to access the slug, TS identifies this as string | number, causing some warnings down the line.

Using pnpm, version 1.5.3, platform: mac

Screenshot 2022-10-28 at 9 56 35 PM

EDIT: As far as I can see the type declaration is

export declare type Params = Record<string, string | number | undefined>;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants