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: remove reactive_declaration_non_reactive_property warning #14111

Closed
wants to merge 7 commits into from

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Nov 1, 2024

closes #14190
closes #13811
closes #14034

Copy link

changeset-bot bot commented Nov 1, 2024

🦋 Changeset detected

Latest commit: 02f469b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

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

Copy link

pkg-pr-new bot commented Nov 1, 2024

pnpm add https://pkg.pr.new/svelte@14111

commit: 3c0d0af

@Rich-Harris
Copy link
Member

I wonder if we can be smarter about when this warning appears — it's not like we didn't add it for a reason (#9287)

@benmccann
Copy link
Member Author

Dominik's point that the code will change behavior when upgrading to runes mode was also a good point. Maybe we just need to add some explanation to the docs about why we have this warning because I couldn't see the reason for it previously

@dummdidumm
Copy link
Member

dummdidumm commented Nov 6, 2024

I think that the cure is worse than the disease, and therefore think we should remove it. Even if the warning had no false positives, you gotta read between the lines to understand what it means.

The only way to keep it I see it to transform it into a runtime warning: If the warning is coming from something else than a reactive declaration or prop, then warn at runtime if we detect that it's a state proxy.

@trueadm
Copy link
Contributor

trueadm commented Nov 6, 2024

Yeah, I think this warning can be removed now. If we have a better ideas for the warning we can always do it in a follow up.

@benmccann
Copy link
Member Author

Even if the warning had no false positives, you gotta read between the lines to understand what it means.

Yeah, that's a big issue with the current warning. I sent #14190 as an alternative which clarifies the message.

I don't have a strong feeling as to whether that's a better solution or this is. I'd be fine to try it out with the clearer message and see if it still annoys people or to just go ahead and remove it

@Rich-Harris
Copy link
Member

If we remove it, #9287 will happen again. We need to come up with a smarter solution

@benmccann
Copy link
Member Author

Closing in favor of #14192

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants