-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Move client types into Astro #3851
Conversation
🦋 Changeset detectedLatest commit: f8e771e The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, however a docs update is needed. The types are referenced on the following pages :
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking' good! All the examples still say "// Add type definitions for our Vite runtime." above astro/client
, though. Would remove or rephrase that.
Agree with @Princesseuh about making sure the docs get updated. Also roping in @tony-sull to comment on the image types.
// images | ||
declare module '*.jpg' { | ||
const src: string | ||
export default src | ||
} | ||
declare module '*.jpeg' { | ||
const src: string | ||
export default src | ||
} | ||
declare module '*.png' { | ||
const src: string | ||
export default src | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means @tony-sull can override these now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By override do you mean change them, then sure. If you mean override in his project, I'm not enough of a TS guru to know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think typescript will let @astrojs/image
re-declare these, but I'm not 100% sure. @matthewp the main need is for @astrojs/image
to be able to define all image imports to something similar to
declare module '*.jpeg' {
const metadata: {
src: string
width: number
height: number
format: string
}
export default metadata
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe adding these to a @astrojs/image/client.d.ts
file and then adding that to compilerOptions.include
will override these. We don't want this to reference the @astrojs/image
types unless @astrojs/image
is installed.
Docs updated in this PR: withastro/docs#953 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's my type
of PR!
* Move client types into Astro * Adds a changeset * Fix path to local client * Reference vite/client in our HMR types * Add back in the expect-error * Update types comment
Changes
vite/client
withastro/client
.astro/client
doesn't contain DOM references, so you don't get weird DOM types in your frontmatter.Testing
Docs