Skip to content

fix: mark accessors and immutable as deprecated #11277

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 20 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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/tiny-meals-deliver.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: mark `accessors` and `immutable` as deprecated
18 changes: 15 additions & 3 deletions packages/svelte/src/compiler/phases/2-analyze/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ export function analyze_component(root, source, options) {
inject_styles: options.css === 'injected' || options.customElement,
accessors: options.customElement
? true
: !!options.accessors ||
: (runes ? false : !!options.accessors) ||
// because $set method needs accessors
!!options.legacy?.componentApi,
reactive_statements: new Map(),
Expand All @@ -406,8 +406,20 @@ export function analyze_component(root, source, options) {
source
};

if (!options.customElement && root.options?.customElement) {
warn(analysis.warnings, root.options, [], 'missing-custom-element-compile-option');
if (root.options) {
for (const attribute of root.options.attributes) {
if (attribute.name === 'accessors') {
warn(analysis.warnings, attribute, [], 'deprecated-accessors');
}

if (attribute.name === 'customElement' && !options.customElement) {
warn(analysis.warnings, attribute, [], 'missing-custom-element-compile-option');
}

if (attribute.name === 'immutable') {
warn(analysis.warnings, attribute, [], 'deprecated-immutable');
}
}
}

if (analysis.runes) {
Expand Down
2 changes: 2 additions & 0 deletions packages/svelte/src/compiler/types/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ export interface CompileOptions extends ModuleCompileOptions {
* If `true`, getters and setters will be created for the component's props. If `false`, they will only be created for readonly exported values (i.e. those declared with `const`, `class` and `function`). If compiling with `customElement: true` this option defaults to `true`.
*
* @default false
* @deprecated This will have no effect in runes mode
*/
accessors?: boolean;
/**
Expand All @@ -107,6 +108,7 @@ export interface CompileOptions extends ModuleCompileOptions {
* This allows it to be less conservative about checking whether values have changed.
*
* @default false
* @deprecated This will have no effect in runes mode
*/
immutable?: boolean;
/**
Expand Down
7 changes: 5 additions & 2 deletions packages/svelte/src/compiler/validate-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ export const validate_component_options =
object({
...common,

accessors: boolean(false),
accessors: deprecate(
'The accessors option has been deprecated. It will have no effect in runes mode.',
boolean(false)
),

css: validator('external', (input) => {
if (input === true || input === false) {
Expand Down Expand Up @@ -73,7 +76,7 @@ export const validate_component_options =
discloseVersion: boolean(true),

immutable: deprecate(
'The immutable option has been deprecated. It has no effect in runes mode.',
'The immutable option has been deprecated. It will have no effect in runes mode.',
boolean(false)
),

Expand Down
6 changes: 5 additions & 1 deletion packages/svelte/src/compiler/warnings.js
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,11 @@ const legacy = {
`Using <slot> to render parent content is deprecated. Use {@render ...} tags instead.`,
/** @param {string} name */
'deprecated-event-handler': (name) =>
`Using on:${name} to listen to the ${name} event is is deprecated. Use the event attribute on${name} instead.`
`Using on:${name} to listen to the ${name} event is is deprecated. Use the event attribute on${name} instead.`,
'deprecated-accessors': () =>
`The accessors option has been deprecated. It will have no effect in runes mode.`,
'deprecated-immutable': () =>
`The immutable option has been deprecated. It will have no effect in runes mode.`
};

const block = {
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/internal/client/render.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ export function mount(component, options) {
* events?: { [Property in keyof Events]: (e: Events[Property]) => any };
* context?: Map<any, any>;
* intro?: boolean;
* recover?: false;
* recover?: boolean;
* }} options
* @returns {Exports}
*/
Expand Down
49 changes: 35 additions & 14 deletions packages/svelte/tests/runtime-legacy/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { setImmediate } from 'node:timers/promises';
import glob from 'tiny-glob/sync.js';
import * as $ from 'svelte/internal/client';
import { createClassComponent } from 'svelte/legacy';
import { flushSync } from 'svelte';
import { flushSync, hydrate, mount, unmount } from 'svelte';
import { render } from 'svelte/server';
import { afterAll, assert, beforeAll } from 'vitest';
import { compile_directory } from '../helpers.js';
Expand Down Expand Up @@ -119,7 +119,7 @@ export function runtime_suite(runes: boolean) {
return common_setup(cwd, runes, config);
},
async (config, cwd, variant, common) => {
await run_test_variant(cwd, config, variant, common);
await run_test_variant(cwd, config, variant, common, runes);
}
);
}
Expand Down Expand Up @@ -147,7 +147,8 @@ async function run_test_variant(
cwd: string,
config: RuntimeTest,
variant: 'dom' | 'hydrate' | 'ssr',
compileOptions: CompileOptions
compileOptions: CompileOptions,
runes: boolean
) {
let unintended_error = false;

Expand Down Expand Up @@ -256,15 +257,30 @@ async function run_test_variant(
}
};

const instance = createClassComponent({
component: mod.default,
props: config.props,
target,
immutable: config.immutable,
intro: config.intro,
recover: config.recover ?? false,
hydrate: variant === 'hydrate'
});
let instance: any;
let props: any;

if (runes) {
props = $.proxy({ ...(config.props || {}) });

const render = variant === 'hydrate' ? hydrate : mount;
instance = render(mod.default, {
target,
props,
intro: config.intro,
recover: config.recover ?? false
});
} else {
instance = createClassComponent({
component: mod.default,
props: config.props,
target,
immutable: config.immutable,
intro: config.intro,
recover: config.recover ?? false,
hydrate: variant === 'hydrate'
});
}

// eslint-disable-next-line no-console
console.warn = warn;
Expand Down Expand Up @@ -303,7 +319,7 @@ async function run_test_variant(
htmlEqualWithOptions: assert_html_equal_with_options
},
variant,
component: instance,
component: runes ? props : instance,
Copy link
Member

Choose a reason for hiding this comment

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

this will surprise us down the line when we want to invoke actual exports on the instance. We should merge the objects instead

Copy link
Member

Choose a reason for hiding this comment

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

Adjusted that

Copy link
Member Author

Choose a reason for hiding this comment

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

Surely a better fix would be to pass them in separately? component eventually gets renamed to props, and there's an additional instance property passed to tests.

Copy link
Member

@dummdidumm dummdidumm Apr 24, 2024

Choose a reason for hiding this comment

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

The adjustments also ensure that I don't have to wrap every usage site with flushSync which I think is better.
I don't care if it's one or two properties.

Copy link
Member Author

Choose a reason for hiding this comment

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

Until you do something like component.todos.push(...), at which point you're in the uncanny valley (because it doesn't invoke the set trap which calls flushSync). Merging them is a kludge. Really, I've come to the view that we shouldn't be setting props in the tests anyway, because things are so much easier to debug if you can just copy-paste stuff into the playground. I'd much prefer that we work towards having a clear distinction (even if we're stymied by legacy mode for the forseeable future) rather than having layers of magic around our tests

mod,
target,
snapshot,
Expand All @@ -318,7 +334,12 @@ async function run_test_variant(
assert.fail('Expected a runtime error');
}
} finally {
instance.$destroy();
if (runes) {
unmount(instance);
} else {
instance.$destroy();
}

assert_html_equal(
target.innerHTML,
'',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import { test } from '../../test';

export default test({
get props() {
return {
items: [{ src: 'https://ds' }]
};
},

async test({ assert, target, component }) {
assert.equal(target.querySelector('img'), component.items[0].img);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<script>
let { items = $bindable([{ src: 'https://ds' }]) } = $props();
let { items } = $props();
</script>

{#each items as item, i}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { flushSync } from 'svelte';
import { test } from '../../test';
import { log } from './log.js';

Expand Down Expand Up @@ -32,7 +33,7 @@ export default test({

log.length = 0;

component.n += 1;
flushSync(() => (component.n += 1));

assert.deepEqual(log, [
'parent: $effect.pre 1',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { flushSync } from 'svelte';
import { test } from '../../test';
import { log } from './log.js';

Expand Down Expand Up @@ -27,7 +28,7 @@ export default test({

log.length = 0;

component.n += 1;
flushSync(() => (component.n += 1));

assert.deepEqual(log, [
'parent: render 1',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { flushSync } from 'svelte';
import { test } from '../../test';
import { log } from './log.js';

Expand Down Expand Up @@ -32,7 +33,7 @@ export default test({

log.length = 0;

component.n += 1;
flushSync(() => (component.n += 1));

assert.deepEqual(log, [
'parent: $effect.pre 1',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { flushSync } from 'svelte';
import { test } from '../../test';
import { log } from './log.js';

Expand All @@ -14,7 +15,7 @@ export default test({
assert.deepEqual(log, ['$effect.pre 0', 'another $effect.pre 1', 'render n0', 'render i1']);

log.length = 0;
component.n += 1;
flushSync(() => (component.n += 1));

assert.deepEqual(log, ['$effect.pre 1', 'another $effect.pre 2', 'render n1', 'render i2']);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { flushSync } from 'svelte';
import { test } from '../../test';
import { log } from './log.js';

Expand Down Expand Up @@ -28,7 +29,7 @@ export default test({

log.length = 0;

component.n += 1;
flushSync(() => (component.n += 1));

assert.deepEqual(log, [
'parent: $effect.pre 1',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { flushSync } from '../../../../src/index-client.js';
import { test } from '../../test';

// Tests that default values only fire lazily when the prop is undefined, and every time
Expand All @@ -8,22 +9,32 @@ export default test({
p2: 0,
p3: 0
},

html: `<p>props: 0 0 0 0 1 1 1 1</p><p>log: nested.fallback_value,fallback_fn`,

async test({ assert, target, component }) {
component.p0 = undefined;
component.p1 = undefined;
component.p2 = undefined;
component.p3 = undefined;
// Nuance: these are already undefined in the props, but we're setting them to undefined again,
// which calls the fallback value again, even if it will result in the same value. There's no way
// to prevent this, and in practise it won't matter - and you shouldn't use accessors in runes mode anyway.
component.p4 = undefined;
component.p5 = undefined;
component.p6 = undefined;
component.p7 = undefined;
flushSync(() => {
component.p0 = undefined;
component.p1 = undefined;
component.p2 = undefined;
component.p3 = undefined;
});

assert.htmlEqual(
target.innerHTML,
`<p>props: 1 1 1 1 1 1 1 1</p><p>log: nested.fallback_value,fallback_fn,nested.fallback_value,fallback_fn`
);

flushSync(() => {
component.p4 = undefined;
component.p5 = undefined;
component.p6 = undefined;
component.p7 = undefined;
});

assert.htmlEqual(
target.innerHTML,
`<p>props: 1 1 1 1 1 1 1 1</p><p>log: nested.fallback_value,fallback_fn,nested.fallback_value,fallback_fn,nested.fallback_value,fallback_fn`
`<p>props: 1 1 1 1 1 1 1 1</p><p>log: nested.fallback_value,fallback_fn,nested.fallback_value,fallback_fn`
);
}
});
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { flushSync } from 'svelte';
import { test } from '../../test';

export default test({
Expand All @@ -8,7 +9,7 @@ export default test({
html: `hello`,

async test({ assert, target, component }) {
component['kebab-case'] = 'goodbye';
flushSync(() => (component['kebab-case'] = 'goodbye'));
assert.htmlEqual(target.innerHTML, `goodbye`);
}
});
7 changes: 4 additions & 3 deletions packages/svelte/tests/runtime-runes/samples/props/_config.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { flushSync } from 'svelte';
import { test } from '../../test';

export default test({
Expand All @@ -9,14 +10,14 @@ export default test({
default2: undefined
};
},

html: `x 1 2 3 z`,

async test({ assert, target, component }) {
component.foo = 'y';
flushSync(() => (component.foo = 'y'));
assert.htmlEqual(target.innerHTML, `y 1 2 3 z`);

// rest props don't generate accessors, so we need to use $set
await component.$set({ bar: 'w' });
flushSync(() => (component.bar = 'w'));
assert.htmlEqual(target.innerHTML, `y 1 2 3 w`);
}
});
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { flushSync } from '../../../../src/index-client.js';
import { test } from '../../test';

export default test({
test({ assert, component, target }) {
const div = /** @type {HTMLDivElement & { foo?: number }} */ (target.querySelector('div'));

assert.equal(div.foo, undefined);
component.foo = 2;
component.visible = false;
flushSync(() => {
component.foo = 2;
component.visible = false;
});
assert.equal(div.foo, 2);
}
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
"message": "The 'customElement' option is used when generating a custom element. Did you forget the 'customElement: true' compile option?",
"start": {
"line": 1,
"column": 0
"column": 16
},
"end": {
"line": 1,
"column": 49
"column": 46
}
}
]
Loading
Loading