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 all 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
2 changes: 1 addition & 1 deletion packages/svelte/messages/compile-warnings/legacy.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@

## deprecated_event_handler

> 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.
4 changes: 4 additions & 0 deletions packages/svelte/messages/compile-warnings/options.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## options_deprecated_accessors

The `accessors` option has been deprecated. It will have no effect in runes mode

## options_deprecated_immutable

> The `immutable` option has been deprecated. It will have no effect in runes mode
Expand Down
11 changes: 8 additions & 3 deletions packages/svelte/scripts/process-messages/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ for (const category of fs.readdirSync('messages')) {
for (const file of fs.readdirSync(`messages/${category}`)) {
if (!file.endsWith('.md')) continue;

const markdown = fs.readFileSync(`messages/${category}/${file}`, 'utf-8');
const markdown = fs
.readFileSync(`messages/${category}/${file}`, 'utf-8')
.replace(/\r\n/g, '\n');

for (const match of markdown.matchAll(/## ([\w]+)\n\n([^]+?)(?=$|\n\n## )/g)) {
const [_, code, text] = match;
Expand All @@ -33,7 +35,9 @@ for (const category of fs.readdirSync('messages')) {
}

function transform(name, dest) {
const source = fs.readFileSync(new URL(`./templates/${name}.js`, import.meta.url), 'utf-8');
const source = fs
.readFileSync(new URL(`./templates/${name}.js`, import.meta.url), 'utf-8')
.replace(/\r\n/g, '\n');

const comments = [];

Expand Down Expand Up @@ -217,7 +221,8 @@ function transform(name, dest) {

fs.writeFileSync(
dest,
`/* This file is generated by scripts/process-messages.js. Do not edit! */\n\n` + module.code,
`/* This file is generated by scripts/process-messages/index.js. Do not edit! */\n\n` +
module.code,
'utf-8'
);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/errors.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* This file is generated by scripts/process-messages.js. Do not edit! */
/* This file is generated by scripts/process-messages/index.js. Do not edit! */

/** @typedef {{ start?: number, end?: number }} NodeLike */
// interface is duplicated between here (used internally) and ./interfaces.js
Expand Down
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 @@ -374,7 +374,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 @@ -395,8 +395,20 @@ export function analyze_component(root, source, options) {
source
};

if (!options.customElement && root.options?.customElement) {
w.options_missing_custom_element(root.options);
if (root.options) {
for (const attribute of root.options.attributes) {
if (attribute.name === 'accessors') {
w.options_deprecated_accessors(attribute);
}

if (attribute.name === 'customElement' && !options.customElement) {
w.options_missing_custom_element(attribute);
}

if (attribute.name === 'immutable') {
w.options_deprecated_immutable(attribute);
}
}
}

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
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/validate-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export const validate_component_options =
object({
...common,

accessors: boolean(false),
accessors: deprecate(w.options_deprecated_accessors, boolean(false)),

css: validator('external', (input) => {
if (input === true || input === false) {
Expand Down
10 changes: 9 additions & 1 deletion packages/svelte/src/compiler/warnings.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* This file is generated by scripts/process-messages.js. Do not edit! */
/* This file is generated by scripts/process-messages/index.js. Do not edit! */

import { getLocator } from 'locate-character';

Expand Down Expand Up @@ -544,6 +544,14 @@ export function invalid_self_closing_tag(node, name) {
w(node, "invalid_self_closing_tag", `Self-closing HTML tags for non-void elements are ambiguous — use <${name} ...></${name}> rather than <${name} ... />`);
}

/**
* e `accessors` option has been deprecated. It will have no effect in runes mode
* @param {null | NodeLike} node
*/
export function options_deprecated_accessors(node) {
w(node, "options_deprecated_accessors", "e `accessors` option has been deprecated. It will have no effect in runes mode");
}

/**
* The `immutable` option has been deprecated. It will have no effect in runes mode
* @param {null | NodeLike} node
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/internal/client/errors.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* This file is generated by scripts/process-messages.js. Do not edit! */
/* This file is generated by scripts/process-messages/index.js. Do not edit! */

import { DEV } from 'esm-env';

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 @@ -119,7 +119,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
2 changes: 1 addition & 1 deletion packages/svelte/src/internal/client/warnings.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* This file is generated by scripts/process-messages.js. Do not edit! */
/* This file is generated by scripts/process-messages/index.js. Do not edit! */

import { DEV } from 'esm-env';

Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/internal/shared/warnings.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* This file is generated by scripts/process-messages.js. Do not edit! */
/* This file is generated by scripts/process-messages/index.js. Do not edit! */

import { DEV } from 'esm-env';

Expand Down
6 changes: 5 additions & 1 deletion packages/svelte/tests/runtime-browser/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,10 @@ async function run_test(

write(`${test_dir}/_output/client/${path.basename(args.path)}.js`, compiled.js.code);

compiled.warnings.forEach((warning) => warnings.push(warning));
compiled.warnings.forEach((warning) => {
if (warning.code === 'options_deprecated_accessors') return;
warnings.push(warning);
});

if (compiled.css !== null) {
compiled.js.code += `document.head.innerHTML += \`<style>${compiled.css.code}</style>\``;
Expand Down Expand Up @@ -179,6 +182,7 @@ async function run_test(
);
} else if (warnings.length) {
/* eslint-disable no-unsafe-finally */
console.warn(warnings);
throw new Error('Received unexpected warnings');
}
}
Expand Down
51 changes: 37 additions & 14 deletions packages/svelte/tests/runtime-legacy/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ import * as fs from 'node:fs';
import { setImmediate } from 'node:timers/promises';
import glob from 'tiny-glob/sync.js';
import { createClassComponent } from 'svelte/legacy';
import { flushSync } from 'svelte';
import { proxy } from 'svelte/internal/client';
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 @@ -43,6 +44,7 @@ export interface RuntimeTest<Props extends Record<string, any> = Record<string,
component: Props & {
[key: string]: any;
};
instance: Record<string, any>;
mod: any;
ok: typeof ok;
raf: {
Expand Down Expand Up @@ -120,7 +122,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 @@ -148,7 +150,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 @@ -289,15 +292,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.error = error;
Expand Down Expand Up @@ -327,7 +345,8 @@ 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

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

if (config.warnings) {
assert.deepEqual(warnings, config.warnings);
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';

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

logs.length = 0;

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

assert.deepEqual(logs, [
'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';

export default test({
Expand All @@ -22,7 +23,7 @@ export default test({

logs.length = 0;

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

assert.deepEqual(logs, [
'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';

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

logs.length = 0;

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

assert.deepEqual(logs, [
'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';

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

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

assert.deepEqual(logs, ['$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';

export default test({
Expand All @@ -23,7 +24,7 @@ export default test({

logs.length = 0;

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

assert.deepEqual(logs, [
'parent: $effect.pre 1',
Expand Down
Loading
Loading