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

Filter out Svelte's unknown class prop console warnings #7291

Merged
merged 9 commits into from
Aug 15, 2023

Conversation

kitschpatrol
Copy link
Contributor

Changes

Astro's hydration code passes a class prop to Svelte components, inducing Svelte to log a warning about an unknown prop.

Preempting this by exporting a class prop from the Svelte component isn't a viable workaround since class is a reserved identifier in JS.

This PR implements the console-filtering workaround suggested by @HiDeoo in #5665, borrowing the useConsoleFilter approach from the preact integration.

It would probably be better to generalize the console filtering process so it could be shared across multiple integrations.

Ideally there would be a way to handle this in Svelte, but as was pointed out in the issue thread even they resort to similar hackery in SvelteKit.

Note that you may need to clear your Vite cache to get this change to stick:

rm -rf ./node_modules/.vite/

Testing

  1. Create an Astro + Svelte project:

    npm create astro@latest -- --template framework-svelte
  2. Run the project and observe the browser console's warning output:

    npm run dev

    Console output:

    <Counter> was created with unknown prop 'class'
    
  3. Apply the patch

  4. Clear the vite cache:

    rm -rf ./node_modules/.vite/
  5. Run the project and observe the browser console's (lack of) warning output:

    npm run dev

    Console output:

Docs

This is a non-breaking change that offers a minor quality of life improvement, not sure documentation beyond what's included in the source comments is necessary.

Astro's hydration code passes a `class` prop to Svelte components, inducing Svelte to log a warning about an unknown prop. Preempting this by exporting a `class` prop from the Svelte component isn't a viable workaround since `class` is a reserved identifier in JS.

This PR implements the console-filtering workaround suggested by @HiDeoo in withastro#5665, borrowing the `useConsoleFilter` approach from the [preact integration](https://github.com/withastro/astro/blob/a1c0cbe604c9f91cdc421b5606aab574999eba01/packages/integrations/preact/src/server.ts#L72-L94).

It would probably be better to generalize console filtering so it could be shared across multiple integrations.

Ideally there would be a way to handle this in Svelte, but as was pointed out in the issue thread even they resort to [similar cringe-inducing hackery](https://github.com/sveltejs/kit/blob/master/packages/kit/src/runtime/client/client.js#L1974-L1996) in sveltekit.
@changeset-bot
Copy link

changeset-bot bot commented Jun 4, 2023

🦋 Changeset detected

Latest commit: 6b59b4f

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: svelte Related to Svelte (scope) pkg: integration Related to any renderer integration (scope) labels Jun 4, 2023
@kitschpatrol kitschpatrol changed the title Filter out Svelte's unexpected class prop console warnings Filter out Svelte's unknown class prop console warnings Jun 4, 2023
@natemoo-re
Copy link
Member

I know the console messages are irritating but I don't love console filtering as a general approach... if this is spreading to multiple renderer integrations, maybe we need to rethink how the Svelte renderer works?

@kitschpatrol
Copy link
Contributor Author

Yeah I'm not crazy about console filtering either, but if you decide it's worth it I've updated the PR with an import.meta.env.DEV check as suggested. Seems a little simpler than maintaining an additional client-dev.js file. Looks like DCE is working as expected.

@wackbyte
Copy link
Contributor

wackbyte commented Jul 1, 2023

FYI, you can export a class prop, like so:

let className;
export { className as class };

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.

I'm personally fine with the patching to avoid the confusion in the logs, but will hold off merging before discussing with @natemoo-re first.

@natemoo-re natemoo-re linked an issue Aug 10, 2023 that may be closed by this pull request
1 task
@natemoo-re
Copy link
Member

Sorry for the delay! I'm fine with merging this since it should help avoid confusion, appreciate the PR!

@natemoo-re natemoo-re merged commit 0bf2cca into withastro:main Aug 15, 2023
4 checks passed
This was referenced Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope) pkg: svelte Related to Svelte (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Svelte island does not expect class prop by default, causing warning
4 participants