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: don't throw with nullish actions #13559

Merged
merged 1 commit into from
Dec 3, 2024
Merged

Conversation

paoloricciuti
Copy link
Member

Svelte 5 rewrite

Closes #13555 ...this may need some discussion because i don't know if it was done on purpose but it was simple enough i said, why not.

Also working on this i noticed we are creating an unnecessary thunk where we could just pass the action in (especially since it's not reactive)...should we fix this too?

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Sorry, something went wrong.

Co-authored-by: Oscar Dominguez <dominguez.celada@gmail.com>
Copy link

changeset-bot bot commented Oct 10, 2024

🦋 Changeset detected

Latest commit: e67f796

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

@paoloricciuti
Copy link
Member Author

Did you see #6942 I've linked? If actions aren't reactive, what's the point of supporting nullish actions? That's a noop. If the prop later changes to non-nullish, the action will still be nullish. See https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAA52SUWvEIAzHv4qTg1o47t57bWHfYW_rPVibFpmnRePgKH732dprxzbGGCIm-cf4I3GivVTgaPE6Uc1vQAv6PI70SPE-zo57B4UQfWe8FXOkdMLKEetGN6gACZphUEAqcnDIEVjPlYP88pC5QGn0LvNFimLvdZI406aDnExztEFhtDMKTsoMLOPZkSxqvESSbgG91Y_sBjtwaM2d5XvohyLZmpYlsnmFZCxH-MLU_sLUfmL6L1L7Z6SD1G4EgSw1cs4tz_sIdNl6xEjsHRRrr40WSoq3aooAVZ0Ytik9JSs9GbfsWYpssNvE2iUpEIgD_SbyywoZQv2yFCjPCSVixf9yM53sJXS0QOshXMMHiz-npmoCAAA=

To support components where you might or might not pass the prop.

<script>
    const { action, children, ...rest } = $props();
</script>

<button use:action {...rest}>
    {@render children?.()}
</button>

You can use it

<Button title="look ma no action">click</Button>

Without having to declare a noop

@Prinzhorn
Copy link
Contributor

Fair point, but to me that sounds like a footgun though when we support nullish actions but you cannot update them.
But on the other hand the problem already exists with two actions that are non-nullish and the prop change won't be reflected. So yeah, ignore my comment.

@Rich-Harris Rich-Harris merged commit 3927568 into main Dec 3, 2024
9 checks passed
@Rich-Harris Rich-Harris deleted the fix-nullish-actions branch December 3, 2024 18:49
@github-actions github-actions bot mentioned this pull request Dec 3, 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.

Allow nullish actions
3 participants