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(types): allow other attributes on script component #26290

Closed
wants to merge 5 commits into from
Closed

fix(types): allow other attributes on script component #26290

wants to merge 5 commits into from

Conversation

advaiyalad
Copy link

@advaiyalad advaiyalad commented Jun 18, 2021

This allows for other attributes on the script component (as the docs say) to exist.

NOTE: This is my first PR, so I hope I didn't do anything wrong in the contrib process... I didn't add integration tests, because this is a types-only change, but could add one if required (I would need help though).

fixes #26236

Bug

  • Related issues linked using fixes #number
  • Integration tests added

@deadcoder0904
Copy link

Thank you. Looks good to me!

@deadcoder0904
Copy link

This one can be merged :)

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

The current <script> and <Script> have the same properties due to the extends ScriptHTMLAttributes here.

So if you were setting unknown props on <script> you would get the same TS error as <Script>.

Similarly, this is the same error you would get for <Image> with unknown props.

If for some reason there are props missing, its best to get this changed upstream in the ScriptHTMLAttributes interface via @types/react

@deadcoder0904
Copy link

@styfle So is there no fix to this? I am assuming what you are saying essentially doesn't allow me to use the custom attributes on Script tag that Utterances needs to use.

Any way to work around that as I feel this isn't going to be merged? Or do I need to send a PR here but don't know if it'll be accepted either?

@styfle
Copy link
Member

styfle commented Jul 7, 2021

Yep, DefinitelyTyped is the correct repository 👍

@deadcoder0904
Copy link

Confused 🤔

Do I send this PR there?

@styfle
Copy link
Member

styfle commented Jul 7, 2021

Yes if there is a prop missing from the <script> types but are available in the DOM, you can add it to DefinitelyTyped.

If you want to define custom types, you can extend the internal Next.js type

declare module "next/script" {
  interface Props {
    someNewProp?: string
  }
}

Although now that I see this, we need to fix the interface name in PR #26990 so it will become:

declare module "next/script" {
  interface ScriptProps {
    someNewProp?: string
  }
}

@kodiakhq kodiakhq bot closed this in #26990 Jul 7, 2021
kodiakhq bot pushed a commit that referenced this pull request Jul 7, 2021
This will ensure `next/script` follows the same naming convention as `next/image`. For example:

```js
import Image, { ImageProps } from 'next/image'
import Script, { ScriptProps } from 'next/script'
```

Fixes #26290
flybayer pushed a commit to blitz-js/next.js that referenced this pull request Aug 19, 2021
This will ensure `next/script` follows the same naming convention as `next/image`. For example:

```js
import Image, { ImageProps } from 'next/image'
import Script, { ScriptProps } from 'next/script'
```

Fixes vercel#26290
@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
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.

Custom attributes like repo on Next/Script for using Utterances gives TypeScript error
4 participants