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

Allow setting a signal value inside a computed #525

Merged
merged 1 commit into from
Mar 15, 2024
Merged

Allow setting a signal value inside a computed #525

merged 1 commit into from
Mar 15, 2024

Conversation

jviide
Copy link
Contributor

@jviide jviide commented Mar 13, 2024

This pull request allows setting a signal's value inside a computed. Previously the following code was not allowed and raised an error with the message "Computed cannot have side-effects":

const s = signal(0);

const c = computed(() => {
  s.value = 1;
});

c.value; // Error: Computed cannot have side-effects

This suggested change is based on Slack discussions (TL;DR: there may be legitimate use-cases that the side-effect limitation blocks / the "regular" cycle detection might be enough for preventing runaway modification loops) and submitted here for further evaluation. /cc @developit @marvinhagemeister

This change saves some bytes from the output bundles:

Before this change:
       1481 B: signals-core.js.gz
       1350 B: signals-core.js.br
       1494 B: signals-core.mjs.gz
       1363 B: signals-core.mjs.br
       1488 B: signals-core.module.js.gz
       1352 B: signals-core.module.js.br
       1557 B: signals-core.min.js.gz
       1414 B: signals-core.min.js.br

After this change:
       1439 B: signals-core.js.gz
       1312 B: signals-core.js.br
       1452 B: signals-core.mjs.gz
       1325 B: signals-core.mjs.br
       1448 B: signals-core.module.js.gz
       1315 B: signals-core.module.js.br
       1518 B: signals-core.min.js.gz
       1381 B: signals-core.min.js.br

Copy link

changeset-bot bot commented Mar 13, 2024

🦋 Changeset detected

Latest commit: cb6bdab

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

This PR includes changesets to release 1 package
Name Type
@preact/signals-core Minor

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

netlify bot commented Mar 13, 2024

Deploy Preview for preact-signals-demo ready!

Name Link
🔨 Latest commit cb6bdab
🔍 Latest deploy log https://app.netlify.com/sites/preact-signals-demo/deploys/65f2f9066f41a40008360054
😎 Deploy Preview https://deploy-preview-525--preact-signals-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@marvinhagemeister marvinhagemeister merged commit 62b7a64 into preactjs:main Mar 15, 2024
6 checks passed
@github-actions github-actions bot mentioned this pull request Mar 15, 2024
@jviide jviide deleted the sides branch March 15, 2024 13:33
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.

3 participants