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

Removing use of reserved symbol $ in components #1870

Closed

Conversation

Sahasrara
Copy link
Contributor

This is in anticipation of the release of Svelte 5

This is in anticipation of the release of Svelte 5
Copy link
Contributor

Choose a reason for hiding this comment

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

The code here strikes me as unnecessarily complicated.

The context, or just the store, could be destructured, then an auto-subscription instead of a manual one could be used.

const { currentId, add, update, change } = getContext("ContentSwitcher");
add({ id, text, selected });

$: selected = $currentId === id;
const { currentId, ...ctx } = getContext("ContentSwitcher");
ctx.add({ id, text, selected });

$: selected = $currentId === id;

(Unless there is a specific timing-related reason to not use a reactive statement here.)

Comment on lines +19 to +20
const stepsById = derived(steps, (stepStore) =>
stepStore.reduce((a, c) => ({ ...a, [c.id]: c }), {})
Copy link
Contributor

Choose a reason for hiding this comment

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

steps is the store, this is the content.

Only $ on its own is reserved, so in cases like this, I would go with the conventional $steps which is analogous to the name in auto-subscriptions.

@metonym
Copy link
Collaborator

metonym commented Mar 7, 2024

Hey, thanks for this PR.

This came up today regarding interoperability with Svelte 5. I've applied the changes in this commit to #1926, and assigned commit co-authorship to @Sahasrara.

@metonym metonym closed this Mar 7, 2024
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.

4 participants