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

Update compile.ts Deals with Hydration directive error #12078

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Amichaiz
Copy link

@Amichaiz Amichaiz commented Sep 26, 2024

Changes

-The following changes solves issue number 11623.
Closes #11623
-The command npm run build now prints the error message that it printed in Developement.
-Updated compile.ts file in astro\packages\astro\src\vite-plugin-astro\compile.ts line 38-46

before
image

after
image

Testing

I recreated the error with the guidence of the issue page. added a Header and called it in the index

recreated both dev and production error. then changed the file compile.ts retested it and now get the error I got in dev mode in production

Docs

I dont belive fixing an error line should need to be in the docs.

Copy link

changeset-bot bot commented Sep 26, 2024

⚠️ No Changeset found

Latest commit: 07a1294

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Sep 26, 2024
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.

Can you please add a patch changeset? pnpm changeset at the root

// Optionally log a warning or error for devs
logger.warn(
null,
`Hydration directive found in ${compileProps.filename}. Astro components should not use client-side rendering.`
Copy link
Member

Choose a reason for hiding this comment

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

Rn we show in what component an invalid client directive is found right? Is it also possible to show on what component it happens?

Copy link
Author

Choose a reason for hiding this comment

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

from my understanding the error is from the way the component is used. and the file that calls it incurrectly that is logged.

@Amichaiz
Copy link
Author

hay, florian-lefebvre its a little late for that... after commiting I deleted it locally.
this is my first time cuntributing to an open source project so sorry if its not done propperly.

@florian-lefebvre
Copy link
Member

No worries! Usually, it's better to keep the repo because we often ask for changes. I'll handle the changeset!

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.

If we want to warn in compile time (which I think is a good thing because we're already doing so in MDX), I think we should warn from here instead:

export async function compileAstro({
compileProps,
astroFileToCompileMetadata,
logger,
}: CompileAstroOption): Promise<CompileAstroResult> {

The transformResult can contain information like this:

{
  "clientOnlyComponents": [],
  "hydratedComponents": [
    {
      "exportName": "default",
      "localName": "",
      "specifier": "./Foo.astro",
      "resolvedPath": "./Foo.astro"
    }
  ]
}

And you can use this information (from clientOnlyComponents and hydratedComponents) to identify if there's misused Astro components by checking the .astro extension in resolvedPath.

This way we can provide more details on which imported component causes it and we can also remove the runtime checks from

// Issue warnings for invalid props for Astro components
function validateComponentProps(props: any, displayName: string) {
if (props != null) {
for (const prop of Object.keys(props)) {
if (prop.startsWith('client:')) {
// eslint-disable-next-line
console.warn(
`You are attempting to render <${displayName} ${prop} />, but ${displayName} is an Astro component. Astro components do not render in the client and should not have a hydration directive. Please use a framework component for client rendering.`,
);
}
}
}
}

(NOTE: you can play with the compiled output at https://live-astro-compiler.vercel.app with advanced settings enabled so it displays a Hydration tab) (looks like there's a bug with localName not showing for default)

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.

Poor error message in production build when client directive used on astro component
4 participants