Skip to content

breaking: rename svelte/reactivity helpers to include Svelte prefix #12248

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

Merged
merged 8 commits into from
Jul 1, 2024

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Jul 1, 2024

After discussing this topic internally, we agree that we should rename the reactivity helpers. Rather than having a Reactive* prefix we instead decided on Svelte* prefix, as it would make it clearer that these are specifically for Svelte.

  • Map becomes SvelteMap
  • Set becomes SvelteSet
  • Date becomes SvelteDate
  • URL becomes SvelteURL

Closes #12192

Copy link

changeset-bot bot commented Jul 1, 2024

🦋 Changeset detected

Latest commit: 5c553ce

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

@Rich-Harris
Copy link
Member

We should keep the old exports around for now, mark them as deprecated, and have them throw a useful error when you try to use them. Otherwise this will be a cryptic breaking change for existing users of these classes

Copy link
Member

@Conduitry Conduitry left a comment

Choose a reason for hiding this comment

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

We can mark the exports as deprecated for the benefit of intellisense, but we shouldn't use the term "deprecated" in the error messages that are thrown - these aren't deprecated, they don't work at all anymore.

@trueadm
Copy link
Contributor Author

trueadm commented Jul 1, 2024

We can mark the exports as deprecated for the benefit of intellisense, but we shouldn't use the term "deprecated" in the error messages that are thrown - these aren't deprecated, they don't work at all anymore.

Good point. tweaked.

@trueadm trueadm merged commit cae5f1e into main Jul 1, 2024
9 checks passed
@trueadm trueadm deleted the rename-reactivity-helpers branch July 1, 2024 18:19
@joshnuss
Copy link
Contributor

joshnuss commented Aug 7, 2024

Just a thought: Would you consider prefixing them with $ instead of Svelte?

For example $Map(), $Set()

That would keep the prefix in line with other runes

@trueadm
Copy link
Contributor Author

trueadm commented Aug 7, 2024

Just a thought: Would you consider prefixing them with $ instead of Svelte?

For example $Map(), $Set()

That would keep the prefix in line with other runes

They aren't runes though?

@joshnuss
Copy link
Contributor

joshnuss commented Aug 7, 2024

Sorry incorrect wording, I meant to keep the naming similar to other reactivity primitives.

First contact with the naming felt slightly odd to me (though to be fair, that happens sometimes with new things)

I feel it would be easier to explain that "a Map isn't reactive, but a $Map is reactive", every time you see a dollar sign, it's reactive.

Also prefixing objects with the name of the framework just felt funny to me. (just an opinion, trying to help keep things simple)

@trueadm
Copy link
Contributor Author

trueadm commented Aug 7, 2024

@joshnuss That's not consistent though. If you create a custom class that has reactive fields, it doesn't have a dollar prefix either, even if it is reactive. The dollar prefix is for runes only (and I guess stores) and thus is a compiler hint that something is special – it does not mean that something is reactive. So, I don't think adding a prefix to Map or Set etc will benefit anything here, just make things more complicated.

@joshnuss
Copy link
Contributor

joshnuss commented Aug 7, 2024

Ok no worries!
Thanks for your time

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

Successfully merging this pull request may close these issues.

The exported names of the reactive Map, Set, and others conflict with platform globals
4 participants