-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix: mark accessors
and immutable
as deprecated
#11277
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
Conversation
🦋 Changeset detectedLatest commit: 2bad3f3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@@ -303,7 +319,7 @@ async function run_test_variant( | |||
htmlEqualWithOptions: assert_html_equal_with_options | |||
}, | |||
variant, | |||
component: instance, | |||
component: runes ? props : instance, |
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.
this will surprise us down the line when we want to invoke actual exports on the instance. We should merge the objects instead
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.
Adjusted that
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.
Surely a better fix would be to pass them in separately? component
eventually gets renamed to props, and there's an additional instance
property passed to tests.
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.
The adjustments also ensure that I don't have to wrap every usage site with flushSync
which I think is better.
I don't care if it's one or two properties.
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.
Until you do something like component.todos.push(...)
, at which point you're in the uncanny valley (because it doesn't invoke the set
trap which calls flushSync
). Merging them is a kludge. Really, I've come to the view that we shouldn't be setting props in the tests anyway, because things are so much easier to debug if you can just copy-paste stuff into the playground. I'd much prefer that we work towards having a clear distinction (even if we're stymied by legacy mode for the forseeable future) rather than having layers of magic around our tests
… code, don't set accessors in runes mode
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.
Made a few changes to the test suite, looks good from my side
…o shared code, don't set accessors in runes mode" This reverts commit a47827e.
immutable
was already deprecated insofar as you'd get a message during options validation (which, incidentally, isn't ideal — it's treated separately from all other compiler warnings. need to come up with a better system. i have some ideas), but it wasn't deprecated in theCompileOptions
types. This fixes that.It also marks
accessors
as deprecated, and makes it not do anything in runes mode, which will allow us to remove it in Svelte 6.A couple of other things that fell out of this:
script
andsvelte:options
attributes in ast #11241, we can warn on individual attributes on<svelte:options>
, so in addition to warning if encounteringaccessors
orimmutable
there, we improve thecustomElement
warningruntime-runes
tests relied onaccessors
. Inrunes
mode, thecomponent
object passed intotest
functions is now a proxy, and the tests themselves have been updated to wrap prop changes influshSync
Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.Tests and linting
pnpm test
and lint the project withpnpm lint