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.

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

Jump to bottom

feat: allow for literal property definition with state on classes #11326

Merged
merged 8 commits into from Apr 29, 2024
Merged

feat: allow for literal property definition with state on classes #11326

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d6d1a26
feat: allow for literal property definition with state on classes
paoloricciuti Apr 24, 2024
15a7cb0
Create honest-nails-share.md
paoloricciuti Apr 24, 2024
ef5a0df
Update honest-nails-share.md
paoloricciuti Apr 24, 2024
ec329a9
chore: rename class in test
paoloricciuti Apr 24, 2024
4136033
fix: replaceAll and use regex
paoloricciuti Apr 25, 2024
d6484a5
Update honest-nails-share.md
paoloricciuti Apr 25, 2024
c7475d6
Merge branch 'main' into literal-property-definition
dummdidumm Apr 29, 2024
99d3a06
simplify
dummdidumm Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/honest-nails-share.md
View file
Open in desktop
@@ -0,0 +1,5 @@
---
"svelte": minor
---

feat: allow for literal property definition with state on classes
241 changes: 137 additions & 104 deletions packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js
View file
Open in desktop
Expand Up @@ -17,6 +17,44 @@ export const javascript_visitors_runes = {
/** @type {string[]} */
const private_ids = [];

/**
*
* @param {import("estree").PropertyDefinition} definition
* @param {boolean} is_private
* @param {string} name
*/
function create_state_field(definition, is_private, name) {
if (definition.value?.type === 'CallExpression') {
const rune = get_rune(definition.value, state.scope);
if (
rune === '$state' ||
rune === '$state.frozen' ||
rune === '$derived' ||
rune === '$derived.by'
) {
/** @type {import('../types.js').StateField} */
const field = {
kind:
rune === '$state'
? 'state'
: rune === '$state.frozen'
? 'frozen_state'
: rune === '$derived.by'
? 'derived_call'
: 'derived',
// @ts-expect-error this is set in the next pass
id: is_private ? definition.key : null
};

if (is_private) {
private_state.set(name, field);
} else {
public_state.set(name, field);
}
}
}
}

for (const definition of node.body) {
if (
definition.type === 'PropertyDefinition' &&
Expand All @@ -27,35 +65,10 @@ export const javascript_visitors_runes = {
const is_private = type === 'PrivateIdentifier';
if (is_private) private_ids.push(name);

if (definition.value?.type === 'CallExpression') {
const rune = get_rune(definition.value, state.scope);
if (
rune === '$state' ||
rune === '$state.frozen' ||
rune === '$derived' ||
rune === '$derived.by'
) {
/** @type {import('../types.js').StateField} */
const field = {
kind:
rune === '$state'
? 'state'
: rune === '$state.frozen'
? 'frozen_state'
: rune === '$derived.by'
? 'derived_call'
: 'derived',
// @ts-expect-error this is set in the next pass
id: is_private ? definition.key : null
};

if (is_private) {
private_state.set(name, field);
} else {
public_state.set(name, field);
}
}
}
create_state_field(definition, is_private, name);
} else if (definition.type === 'PropertyDefinition' && definition.key.type === 'Literal') {
const name = definition.key.value?.toString().replace('-', '_');
Copy link
Member

Choose a reason for hiding this comment

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

Don't think this is enough - there could be all sorts of whacky characters in there, better to use a "non-valid ID identifier" regex (I believe there exists one already)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh great, yesterday I started to think of other invalid characters but nothing come to mind...I'll update soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also just realized I need replaceAll

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dummdidumm fixed it in the new commit...should i add more tests for all the same thing that were tested with normal identifiers?

if (name) create_state_field(definition, false, name);
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand All @@ -75,6 +88,95 @@ export const javascript_visitors_runes = {

const child_state = { ...state, public_state, private_state };

/**
*
* @param {import("estree").PropertyDefinition} definition
* @param {boolean} is_private
* @param {string} name
*/
function replace_class_body(definition, is_private, name) {
const field = (is_private ? private_state : public_state).get(name);

if (definition.value?.type === 'CallExpression' && field !== undefined) {
let value = null;

if (definition.value.arguments.length > 0) {
const init = /** @type {import('estree').Expression} **/ (
visit(definition.value.arguments[0], child_state)
);

value =
field.kind === 'state'
? b.call(
'$.source',
should_proxy_or_freeze(init, state.scope) ? b.call('$.proxy', init) : init
)
: field.kind === 'frozen_state'
? b.call(
'$.source',
should_proxy_or_freeze(init, state.scope) ? b.call('$.freeze', init) : init
)
: field.kind === 'derived_call'
? b.call('$.derived', init)
: b.call('$.derived', b.thunk(init));
} else {
// if no arguments, we know it's state as `$derived()` is a compile error
value = b.call('$.source');
}

if (is_private) {
body.push(b.prop_def(field.id, value));
} else {
// #foo;
const member = b.member(b.this, field.id);
body.push(b.prop_def(field.id, value));

// get foo() { return this.#foo; }
body.push(b.method('get', definition.key, [], [b.return(b.call('$.get', member))]));

if (field.kind === 'state') {
// set foo(value) { this.#foo = value; }
const value = b.id('value');
body.push(
b.method(
'set',
definition.key,
[value],
[b.stmt(b.call('$.set', member, b.call('$.proxy', value)))]
)
);
}

if (field.kind === 'frozen_state') {
// set foo(value) { this.#foo = value; }
const value = b.id('value');
body.push(
b.method(
'set',
definition.key,
[value],
[b.stmt(b.call('$.set', member, b.call('$.freeze', value)))]
)
);
}

if ((field.kind === 'derived' || field.kind === 'derived_call') && state.options.dev) {
body.push(
b.method(
'set',
definition.key,
[b.id('_')],
[b.throw_error(`Cannot update a derived property ('${name}')`)]
)
);
}
}

return true;
}
return false;
}

// Replace parts of the class body
for (const definition of node.body) {
if (
Expand All @@ -84,83 +186,14 @@ export const javascript_visitors_runes = {
const name = definition.key.name;

const is_private = definition.key.type === 'PrivateIdentifier';
const field = (is_private ? private_state : public_state).get(name);

if (definition.value?.type === 'CallExpression' && field !== undefined) {
let value = null;

if (definition.value.arguments.length > 0) {
const init = /** @type {import('estree').Expression} **/ (
visit(definition.value.arguments[0], child_state)
);

value =
field.kind === 'state'
? b.call(
'$.source',
should_proxy_or_freeze(init, state.scope) ? b.call('$.proxy', init) : init
)
: field.kind === 'frozen_state'
? b.call(
'$.source',
should_proxy_or_freeze(init, state.scope) ? b.call('$.freeze', init) : init
)
: field.kind === 'derived_call'
? b.call('$.derived', init)
: b.call('$.derived', b.thunk(init));
} else {
// if no arguments, we know it's state as `$derived()` is a compile error
value = b.call('$.source');
}

if (is_private) {
body.push(b.prop_def(field.id, value));
} else {
// #foo;
const member = b.member(b.this, field.id);
body.push(b.prop_def(field.id, value));

// get foo() { return this.#foo; }
body.push(b.method('get', definition.key, [], [b.return(b.call('$.get', member))]));

if (field.kind === 'state') {
// set foo(value) { this.#foo = value; }
const value = b.id('value');
body.push(
b.method(
'set',
definition.key,
[value],
[b.stmt(b.call('$.set', member, b.call('$.proxy', value)))]
)
);
}

if (field.kind === 'frozen_state') {
// set foo(value) { this.#foo = value; }
const value = b.id('value');
body.push(
b.method(
'set',
definition.key,
[value],
[b.stmt(b.call('$.set', member, b.call('$.freeze', value)))]
)
);
}

if ((field.kind === 'derived' || field.kind === 'derived_call') && state.options.dev) {
body.push(
b.method(
'set',
definition.key,
[b.id('_')],
[b.throw_error(`Cannot update a derived property ('${name}')`)]
)
);
}
}
if (replace_class_body(definition, is_private, name)) {
continue;
}
} else if (definition.type === 'PropertyDefinition' && definition.key.type === 'Literal') {
const name = definition.key.value?.toString().replace('-', '_');

if (name && replace_class_body(definition, false, name)) {
continue;
}
}
Expand Down
15 changes: 15 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/class-state-with-literal/_config.js
View file
Open in desktop
@@ -0,0 +1,15 @@
import { test } from '../../test';

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

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

await btn?.click();
assert.htmlEqual(target.innerHTML, `<button>true</button>`);

await btn?.click();
assert.htmlEqual(target.innerHTML, `<button>false</button>`);
}
});
12 changes: 12 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/class-state-with-literal/main.svelte
View file
Open in desktop
@@ -0,0 +1,12 @@
<script>
class Toggle {
"aria-pressed" = $state(false);

toggle(){
this["aria-pressed"] = !this["aria-pressed"]
}
}
const toggle = new Toggle();
</script>

<button on:click={() => toggle.toggle()}>{toggle["aria-pressed"]}</button>