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: Type error on <Image /> component when strict TypeScript rules are used with library type checking. #10549

Merged
merged 10 commits into from Mar 26, 2024

Conversation

admirsaheta
Copy link
Contributor

Changes

  • What does this change?
    Fixes the following typescript issue, with it now being explicitly defined to avoid type mismatch on the runtime interpreter
Element implicitly has an 'any' type because expression of type '"data-image-component"' can't be used to index type 'HTMLAttributes<"img">'.
  Property 'data-image-component' does not exist on type 'HTMLAttributes<"img">'.

image

image

Testing

Docs

Copy link

changeset-bot bot commented Mar 25, 2024

🦋 Changeset detected

Latest commit: e2f956b

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 pkg: astro Related to the core `astro` package (scope) semver: major Change triggers a `major` release labels Mar 25, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a major changeset. A reviewer will merge this at the next release if approved.

Copy link
Member

@Fryuni Fryuni left a comment

Choose a reason for hiding this comment

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

Hey @admirsaheta, thanks for the PR! We are always glad to have new contributors.

There are just some things to improve on the changeset:

  • Those messages get added to the release notes and to the changelog, so we use clear descriptions of what was changed or fixed for people to understand how it might affect them.
  • The changeset also indicates how the change relates to SemVer, so a "major" there would mean a breaking change that would have to wait until Astro 5 to be included. That is not the case here. Since this PR doesn't change any Astro behavior and fixes a problem it can be a patch.

.changeset/blue-dolls-melt.md Outdated Show resolved Hide resolved
.changeset/blue-dolls-melt.md Outdated Show resolved Hide resolved
@admirsaheta admirsaheta changed the title hotfix:ts-explicit-error Fix: Type error on <Image /> component when strict TypeScript rules are used with library type checking. Mar 25, 2024
@Fryuni Fryuni dismissed github-actions’s stale review March 25, 2024 11:27

Not a major change

@Princesseuh
Copy link
Member

Hmm, I think there's a better fix here. HTMLAttributes not allowing data attributes is a problem

@Fryuni Fryuni removed the semver: major Change triggers a `major` release label Mar 25, 2024
Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
@admirsaheta
Copy link
Contributor Author

Hmm, I think there's a better fix here. HTMLAttributes not allowing data attributes is a problem

You could extend the interface and provide custom attributes there e.g

interface AdditionalAttributes extends HTMLAttributes<HTMLImageElement> {
    'data-image-component'?: string;
}

Let me know which approach fits better

@florian-lefebvre
Copy link
Member

florian-lefebvre commented Mar 25, 2024

We have been discussing on discord to update the HTMLAttributes. So at

export type HTMLAttributes<Tag extends HTMLTag> = Omit<
allow such type:

[key: `data-${string}`]: any

Do you want to handle it?

@admirsaheta
Copy link
Contributor Author

Sure thing, give me a few :)

@florian-lefebvre
Copy link
Member

There's no rush don't worry!

@admirsaheta
Copy link
Contributor Author

Should handle it correctly here now 👍

>;
keyof Omit<AstroBuiltinAttributes, 'class:list'>
> & {
[key: string]: string | number | boolean | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

@Princesseuh Do you prefer any?

Copy link
Member

Choose a reason for hiding this comment

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

Without necessarily using any, allowing also null would be great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without necessarily using any, allowing also null would be great

Done

.changeset/blue-dolls-melt.md Outdated Show resolved Hide resolved
admirsaheta and others added 2 commits March 25, 2024 13:32
Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
Copy link
Member

@florian-lefebvre florian-lefebvre left a comment

Choose a reason for hiding this comment

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

LGTM

@Princesseuh Princesseuh merged commit 54c2f97 into withastro:main Mar 26, 2024
13 checks passed
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.

None yet

4 participants