Skip to content

feat: make fallback prop values readonly #9789

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.

Sign up for GitHub

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
Dec 5, 2023
Merged

feat: make fallback prop values readonly #9789

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5dc9830
WIP
Rich-Harris Dec 5, 2023
c263119
update tests
Rich-Harris Dec 5, 2023
5fdafb1
only make readonly in runes mode
Rich-Harris Dec 5, 2023
ddbc5a9
remove this for now
Rich-Harris Dec 5, 2023
7f97a95
changeset
Rich-Harris Dec 5, 2023
d884812
ugh
Rich-Harris Dec 5, 2023
fce95f0
add reassignment test
Rich-Harris Dec 5, 2023
23064b1
tweak message
Rich-Harris Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/lazy-months-knock.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

feat: make fallback prop values readonly
8 changes: 7 additions & 1 deletion packages/svelte/src/internal/client/runtime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { subscribe_to_store } from '../../store/utils.js';
import { EMPTY_FUNC, run_all } from '../common.js';
import { get_descriptor, get_descriptors, is_array } from './utils.js';
import { PROPS_CALL_DEFAULT_VALUE, PROPS_IS_IMMUTABLE, PROPS_IS_RUNES } from '../../constants.js';
import { readonly } from './proxy/readonly.js';

export const SOURCE = 1;
export const DERIVED = 1 << 1;
Expand Down Expand Up @@ -1422,13 +1423,14 @@ export function is_store(val) {
export function prop_source(props_obj, key, flags, default_value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm starting to think it might make more sense to have two versions of this – one for runes and one for legacy. I'm really unhappy that we have sync_effect as it's an anti-pattern, so if we can somehow get to a point where we kill that in runes, it enables us to make forking of effects work. With sync effects that happen during the notification phase, that's pretty much impossible as it means effects fire and potentially invalidate another effect tree, making forking impossible.

const call_default_value = (flags & PROPS_CALL_DEFAULT_VALUE) !== 0;
const immutable = (flags & PROPS_IS_IMMUTABLE) !== 0;
const runes = (flags & PROPS_IS_RUNES) !== 0;

const props = is_signal(props_obj) ? get(props_obj) : props_obj;
const update_bound_prop = get_descriptor(props, key)?.set;
let value = props[key];
const should_set_default_value = value === undefined && default_value !== undefined;

if (update_bound_prop && default_value !== undefined && (flags & PROPS_IS_RUNES) !== 0) {
if (update_bound_prop && runes && default_value !== undefined) {
// TODO consolidate all these random runtime errors
throw new Error('Cannot use fallback values with bind:');
}
Expand All @@ -1437,6 +1439,10 @@ export function prop_source(props_obj, key, flags, default_value) {
value =
// @ts-expect-error would need a cumbersome method overload to type this
call_default_value ? default_value() : default_value;

if (DEV && runes) {
value = readonly(/** @type {any} */ (value));
}
}

const source_signal = immutable ? source(value) : mutable_source(value);
Expand Down
15 changes: 10 additions & 5 deletions packages/svelte/tests/runtime-runes/samples/class-state-derived-unowned/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,41 @@
import { flushSync } from 'svelte';
import { test } from '../../test';
import { log } from './log.js';

export default test({
// The component context class instance gets shared between tests, strangely, causing hydration to fail?
skip_if_hydrate: 'permanent',

async test({ assert, target, component }) {
before_test() {
log.length = 0;
},

async test({ assert, target }) {
const btn = target.querySelector('button');

flushSync(() => {
btn?.click();
});

assert.deepEqual(component.log, [0, 'class trigger false', 'local trigger false', 1]);
assert.deepEqual(log, [0, 'class trigger false', 'local trigger false', 1]);

flushSync(() => {
btn?.click();
});

assert.deepEqual(component.log, [0, 'class trigger false', 'local trigger false', 1, 2]);
assert.deepEqual(log, [0, 'class trigger false', 'local trigger false', 1, 2]);

flushSync(() => {
btn?.click();
});

assert.deepEqual(component.log, [0, 'class trigger false', 'local trigger false', 1, 2, 3]);
assert.deepEqual(log, [0, 'class trigger false', 'local trigger false', 1, 2, 3]);

flushSync(() => {
btn?.click();
});

assert.deepEqual(component.log, [
assert.deepEqual(log, [
0,
'class trigger false',
'local trigger false',
Expand Down
2 changes: 2 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/class-state-derived-unowned/log.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/** @type {any[]} */
export const log = [];
14 changes: 7 additions & 7 deletions packages/svelte/tests/runtime-runes/samples/class-state-derived-unowned/main.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
<script context="module">
class SomeLogic {
someValue = $state(0);
isAboveThree = $derived(this.someValue > 3);
trigger() {
this.someValue++;
}
class SomeLogic {
someValue = $state(0);
isAboveThree = $derived(this.someValue > 3);
trigger() {
this.someValue++;
}
}

const someLogic = new SomeLogic();
</script>

<script>
const {log = []} = $props();
import { log } from './log.js';

function increment() {
someLogic.trigger();
Expand Down
9 changes: 7 additions & 2 deletions packages/svelte/tests/runtime-runes/samples/class-state-init-eager-2/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import { test } from '../../test';
import { log } from './log.js';

export default test({
html: `<button>0</button>`,

async test({ assert, target, component }) {
before_test() {
log.length = 0;
},

async test({ assert, target }) {
const btn = target.querySelector('button');

await btn?.click();
Expand All @@ -12,6 +17,6 @@ export default test({
await btn?.click();
assert.htmlEqual(target.innerHTML, `<button>2</button>`);

assert.deepEqual(component.log, [undefined]);
assert.deepEqual(log, [undefined]);
}
});
2 changes: 2 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/class-state-init-eager-2/log.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/** @type {any[]} */
export const log = [];
2 changes: 1 addition & 1 deletion packages/svelte/tests/runtime-runes/samples/class-state-init-eager-2/main.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<script>
const {log = []} = $props();
import { log } from './log.js';

const logger = (obj) => {
log.push(obj.count)
Expand Down
9 changes: 7 additions & 2 deletions packages/svelte/tests/runtime-runes/samples/class-state-init-eager-3/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import { test } from '../../test';
import { log } from './log.js';

export default test({
html: `<button>0</button>`,

async test({ assert, target, component }) {
before_test() {
log.length = 0;
},

async test({ assert, target }) {
const btn = target.querySelector('button');

await btn?.click();
Expand All @@ -12,6 +17,6 @@ export default test({
await btn?.click();
assert.htmlEqual(target.innerHTML, `<button>2</button>`);

assert.deepEqual(component.log, [undefined]);
assert.deepEqual(log, [undefined]);
}
});
2 changes: 2 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/class-state-init-eager-3/log.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/** @type {any[]} */
export const log = [];
2 changes: 1 addition & 1 deletion packages/svelte/tests/runtime-runes/samples/class-state-init-eager-3/main.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<script>
const {log = []} = $props();
import { log } from './log.js';

class Counter {
count = $state();
Expand Down
9 changes: 7 additions & 2 deletions packages/svelte/tests/runtime-runes/samples/class-state-init-eager/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import { test } from '../../test';
import { log } from './log.js';

export default test({
html: `<button>0</button>`,

async test({ assert, target, component }) {
before_test() {
log.length = 0;
},

async test({ assert, target }) {
const btn = target.querySelector('button');

await btn?.click();
Expand All @@ -12,6 +17,6 @@ export default test({
await btn?.click();
assert.htmlEqual(target.innerHTML, `<button>2</button>`);

assert.deepEqual(component.log, [100]);
assert.deepEqual(log, [100]);
}
});
2 changes: 2 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/class-state-init-eager/log.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/** @type {any[]} */
export const log = [];
2 changes: 1 addition & 1 deletion packages/svelte/tests/runtime-runes/samples/class-state-init-eager/main.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<script>
const {log = []} = $props();
import { log } from './log.js';

class Counter {
count = $state(100);
Expand Down
9 changes: 7 additions & 2 deletions packages/svelte/tests/runtime-runes/samples/event-attribute-template/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
import { flushSync } from 'svelte';
import { test } from '../../test';
import { log } from './log.js';

export default test({
async test({ assert, target, component }) {
before_test() {
log.length = 0;
},

async test({ assert, target }) {
const [b1] = target.querySelectorAll('button');

flushSync(() => {
b1?.click();
});

await Promise.resolve();
assert.deepEqual(component.log, ['onclick']);
assert.deepEqual(log, ['onclick']);
}
});
2 changes: 2 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/event-attribute-template/log.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/** @type {any[]} */
export const log = [];
2 changes: 1 addition & 1 deletion packages/svelte/tests/runtime-runes/samples/event-attribute-template/main.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<script>
const {log = []} = $props();
import { log } from './log.js';

function send() {
log.push("onclick")
Expand Down
8 changes: 8 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/proxy-prop-default-readonly/Counter.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<script>
/** @type {{ object?: { count: number }}} */
let { object = { count: 0 } } = $props();
</script>

<button onclick={() => object.count += 1}>
clicks: {object.count}
</button>
18 changes: 18 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/proxy-prop-default-readonly/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { test } from '../../test';

export default test({
html: `<button>clicks: 0</button>`,

compileOptions: {
dev: true
},

async test({ assert, target }) {
const btn = target.querySelector('button');
await btn?.click();

assert.htmlEqual(target.innerHTML, `<button>clicks: 0</button>`);
},

runtime_error: 'Props are read-only, unless used with `bind:`'
});
5 changes: 5 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/proxy-prop-default-readonly/main.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<script>
import Counter from './Counter.svelte';
</script>

<Counter />