From 5c6d1110656b2b474cf9161ddd5bcf5656bb25cf Mon Sep 17 00:00:00 2001 From: Tan Li Hau Date: Sat, 27 May 2023 02:49:32 +0800 Subject: [PATCH] Do not expose default slot let bindings to named slots (#6049) * should not extend scope for across slots * disallow named slots inheriting let: scope from default slots * fix tests * fix test * fix * add runtime tests * rename test since it doesn't inherit anymore * fix lint * remove warnings * add compile script * document script * improve warning * fix test * handle renames * fix lint * gather names from all parents instead of just the nearest * remove unused import * add reminder --------- Co-authored-by: gtmnayan --- scripts/compile-test.js | 40 +++++++++++++++++++ src/compiler/compile/Component.js | 31 +++++++++++++- src/compiler/compile/nodes/InlineComponent.js | 40 +++++++++---------- src/compiler/compile/nodes/Slot.js | 34 +--------------- .../compile/nodes/shared/Expression.js | 9 +++-- .../wrappers/InlineComponent/index.js | 28 +++++-------- .../render_dom/wrappers/SlotTemplate.js | 5 +-- .../compile/render_ssr/handlers/Slot.js | 5 --- .../render_ssr/handlers/SlotTemplate.js | 6 +-- .../Nested.svelte | 11 +++++ .../component-slot-default-in-each/_config.js | 7 ++++ .../main.svelte | 7 ++++ .../component-slot-let-scope-2/Nested.svelte | 3 ++ .../component-slot-let-scope-2/_config.js | 3 ++ .../component-slot-let-scope-2/main.svelte | 8 ++++ .../Nested.svelte | 2 - .../_config.js | 4 +- .../main.svelte | 1 + .../component-slot-let-scope/Nested.svelte | 2 + .../component-slot-let-scope/_config.js | 3 ++ .../component-slot-let-scope/main.svelte | 8 ++++ .../component-slot-nested-in-slot/One.svelte | 6 ++- .../main.svelte | 4 +- .../main.svelte | 4 +- .../component-svelte-fragment-let-e/A.svelte | 4 +- .../_config.js | 4 -- .../main.svelte | 6 --- .../samples/lets-on-component/input.svelte | 27 +++++++++++++ .../samples/lets-on-component/warnings.json | 26 ++++++++++++ 29 files changed, 228 insertions(+), 110 deletions(-) create mode 100644 scripts/compile-test.js create mode 100644 test/runtime/samples/component-slot-default-in-each/Nested.svelte create mode 100644 test/runtime/samples/component-slot-default-in-each/_config.js create mode 100644 test/runtime/samples/component-slot-default-in-each/main.svelte create mode 100644 test/runtime/samples/component-slot-let-scope-2/Nested.svelte create mode 100644 test/runtime/samples/component-slot-let-scope-2/_config.js create mode 100644 test/runtime/samples/component-slot-let-scope-2/main.svelte rename test/runtime/samples/{component-slot-named-inherits-default-lets => component-slot-let-scope-3}/Nested.svelte (82%) rename test/runtime/samples/{component-slot-named-inherits-default-lets => component-slot-let-scope-3}/_config.js (84%) rename test/runtime/samples/{component-slot-named-inherits-default-lets => component-slot-let-scope-3}/main.svelte (90%) create mode 100644 test/runtime/samples/component-slot-let-scope/Nested.svelte create mode 100644 test/runtime/samples/component-slot-let-scope/_config.js create mode 100644 test/runtime/samples/component-slot-let-scope/main.svelte create mode 100644 test/validator/samples/lets-on-component/input.svelte create mode 100644 test/validator/samples/lets-on-component/warnings.json diff --git a/scripts/compile-test.js b/scripts/compile-test.js new file mode 100644 index 0000000000..9a83708b38 --- /dev/null +++ b/scripts/compile-test.js @@ -0,0 +1,40 @@ +// Compile all Svelte files in a directory to JS and CSS files +// Usage: node scripts/compile-test.js + +import { mkdirSync, readFileSync, writeFileSync } from 'fs'; +import path from 'path'; +import glob from 'tiny-glob/sync.js'; +import { compile } from '../src/compiler/index.js'; + +const cwd = path.resolve(process.argv[2]); + +const options = [ + ['normal', {}], + ['hydrate', { hydratable: true }], + ['ssr', { generate: 'ssr' }] +]; +for (const file of glob('**/*.svelte', { cwd })) { + const contents = readFileSync(`${cwd}/${file}`, 'utf-8').replace(/\r/g, ''); + let w; + for (const [name, opts] of options) { + const dir = `${cwd}/_output/${name}`; + + const { js, css, warnings } = compile(contents, { + ...opts, + filename: file + }); + + if (warnings.length) { + w = warnings; + } + + mkdirSync(dir, { recursive: true }); + js.code && writeFileSync(`${dir}/${file.replace(/\.svelte$/, '.js')}`, js.code); + css.code && writeFileSync(`${dir}/${file.replace(/\.svelte$/, '.css')}`, css.code); + } + + if (w) { + console.log(`Warnings for ${file}:`); + console.log(w); + } +} diff --git a/src/compiler/compile/Component.js b/src/compiler/compile/Component.js index 7d3615bb82..894cb9b787 100644 --- a/src/compiler/compile/Component.js +++ b/src/compiler/compile/Component.js @@ -1643,8 +1643,9 @@ export default class Component { * @param {string} name * @param {any} node * @param {import('./nodes/shared/TemplateScope.js').default} template_scope + * @param {import("./nodes/shared/Node.js").default} [owner] */ - warn_if_undefined(name, node, template_scope) { + warn_if_undefined(name, node, template_scope, owner) { if (name[0] === '$') { if (name === '$' || (name[1] === '$' && !is_reserved_keyword(name))) { return this.error(node, compiler_errors.illegal_global(name)); @@ -1656,6 +1657,34 @@ export default class Component { if (this.var_lookup.has(name) && !this.var_lookup.get(name).global) return; if (template_scope && template_scope.names.has(name)) return; if (globals.has(name) && node.type !== 'InlineComponent') return; + + function has_out_of_scope_let() { + for (let parent = owner.parent; parent; parent = parent.parent) { + if (parent.type === 'InlineComponent') { + const { let_attributes } = parent; + + for (const attr of let_attributes) { + if ( + // @ts-expect-error + // TODO extract_names only considers patterns but let attributes return expressions + (attr.expression && extract_names(attr.expression).includes(name)) || + attr.name === name + ) + return true; + } + } + } + + return false; + } + + if (owner && has_out_of_scope_let()) { + return this.warn(node, { + code: 'missing-declaration', + message: `let:${name} declared on parent component cannot be used inside named slot` + }); + } + this.warn(node, compiler_warnings.missing_declaration(name, !!this.ast.instance)); } diff --git a/src/compiler/compile/nodes/InlineComponent.js b/src/compiler/compile/nodes/InlineComponent.js index 792b931656..54b0dfb989 100644 --- a/src/compiler/compile/nodes/InlineComponent.js +++ b/src/compiler/compile/nodes/InlineComponent.js @@ -4,7 +4,6 @@ import map_children from './shared/map_children.js'; import Binding from './Binding.js'; import EventHandler from './EventHandler.js'; import Expression from './shared/Expression.js'; -import Let from './Let.js'; import compiler_errors from '../compiler_errors.js'; import { regex_only_whitespaces } from '../../utils/patterns.js'; @@ -22,9 +21,6 @@ export default class InlineComponent extends Node { /** @type {import('./EventHandler.js').default[]} */ handlers = []; - /** @type {import('./Let.js').default[]} */ - lets = []; - /** @type {import('./Attribute.js').default[]} */ css_custom_properties = []; @@ -37,6 +33,8 @@ export default class InlineComponent extends Node { /** @type {string} */ namespace; + /** @type {Attribute[]} */ + let_attributes; /** * @param {import('../Component.js').default} component * @param {import('./shared/Node.js').default} parent @@ -58,6 +56,8 @@ export default class InlineComponent extends Node { this.name === 'svelte:component' ? new Expression(component, this, scope, info.expression) : null; + + const let_attributes = (this.let_attributes = []); info.attributes.forEach( /** @param {import('../../interfaces.js').BaseDirective | import('../../interfaces.js').Attribute | import('../../interfaces.js').SpreadAttribute} node */ ( node @@ -84,7 +84,7 @@ export default class InlineComponent extends Node { this.handlers.push(new EventHandler(component, this, scope, node)); break; case 'Let': - this.lets.push(new Let(component, this, scope, node)); + let_attributes.push(node); break; case 'Transition': return component.error(node, compiler_errors.invalid_transition); @@ -98,21 +98,9 @@ export default class InlineComponent extends Node { /* eslint-enable no-fallthrough */ } ); - if (this.lets.length > 0) { - this.scope = scope.child(); - this.lets.forEach( - /** @param {any} l */ (l) => { - const dependencies = new Set([l.name.name]); - l.names.forEach( - /** @param {any} name */ (name) => { - this.scope.add(name, dependencies, this); - } - ); - } - ); - } else { - this.scope = scope; - } + + this.scope = scope; + this.handlers.forEach( /** @param {any} handler */ (handler) => { handler.modifiers.forEach( @@ -178,6 +166,18 @@ export default class InlineComponent extends Node { children: info.children }); } + + if (let_attributes.length) { + // copy let: attribute from to + // as they are for `slot="default"` only + children.forEach((child) => { + const slot = child.attributes.find((attribute) => attribute.name === 'slot'); + if (!slot || slot.value[0].data === 'default') { + child.attributes.push(...let_attributes); + } + }); + } + this.children = map_children(component, this, this.scope, children); } get slot_template_name() { diff --git a/src/compiler/compile/nodes/Slot.js b/src/compiler/compile/nodes/Slot.js index fc601943ab..bdfb44a8f9 100644 --- a/src/compiler/compile/nodes/Slot.js +++ b/src/compiler/compile/nodes/Slot.js @@ -40,39 +40,7 @@ export default class Slot extends Element { } ); if (!this.slot_name) this.slot_name = 'default'; - if (this.slot_name === 'default') { - // if this is the default slot, add our dependencies to any - // other slots (which inherit our slot values) that were - // previously encountered - component.slots.forEach( - /** @param {any} slot */ (slot) => { - this.values.forEach( - /** - * @param {any} attribute - * @param {any} name - */ (attribute, name) => { - if (!slot.values.has(name)) { - slot.values.set(name, attribute); - } - } - ); - } - ); - } else if (component.slots.has('default')) { - // otherwise, go the other way — inherit values from - // a previously encountered default slot - const default_slot = component.slots.get('default'); - default_slot.values.forEach( - /** - * @param {any} attribute - * @param {any} name - */ (attribute, name) => { - if (!this.values.has(name)) { - this.values.set(name, attribute); - } - } - ); - } + component.slots.set(this.slot_name, this); this.cannot_use_innerhtml(); this.not_static_content(); diff --git a/src/compiler/compile/nodes/shared/Expression.js b/src/compiler/compile/nodes/shared/Expression.js index d617b0b1a7..c4877f7c84 100644 --- a/src/compiler/compile/nodes/shared/Expression.js +++ b/src/compiler/compile/nodes/shared/Expression.js @@ -77,6 +77,7 @@ export default class Expression { this.scope_map = map; const expression = this; let function_expression; + // discover dependencies, but don't change the code yet walk(info, { /** @@ -125,7 +126,7 @@ export default class Expression { dependencies.add(name); } component.add_reference(node, name); - component.warn_if_undefined(name, nodes[0], template_scope); + component.warn_if_undefined(name, nodes[0], template_scope, owner); } this.skip(); } @@ -376,8 +377,9 @@ export default class Expression { node = node.parent; } const func_expression = func_declaration[0]; - if (node.type === 'InlineComponent' || node.type === 'SlotTemplate') { - // + + if (node.type === 'SlotTemplate') { + // this.replace(func_expression); } else { // {#each}, {#await} @@ -458,6 +460,7 @@ export default class Expression { } } }); + if (declarations.length > 0) { block.maintain_context = true; declarations.forEach( diff --git a/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js b/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js index 0aab7fc01b..3544a759dd 100644 --- a/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js +++ b/src/compiler/compile/render_dom/wrappers/InlineComponent/index.js @@ -1,19 +1,18 @@ -import Wrapper from '../shared/Wrapper.js'; -import BindingWrapper from '../Element/Binding.js'; -import SlotTemplateWrapper from '../SlotTemplate.js'; +import { b, p, x } from 'code-red'; +import { extract_ignores_above_node } from '../../../../utils/extract_svelte_ignore.js'; import { sanitize } from '../../../../utils/names.js'; +import { namespaces } from '../../../../utils/namespaces.js'; +import compiler_warnings from '../../../compiler_warnings.js'; import add_to_set from '../../../utils/add_to_set.js'; -import { b, x, p } from 'code-red'; -import is_dynamic from '../shared/is_dynamic.js'; -import bind_this from '../shared/bind_this.js'; -import EventHandler from '../Element/EventHandler.js'; -import { extract_names } from 'periscopic'; -import mark_each_block_bindings from '../shared/mark_each_block_bindings.js'; import { string_to_member_expression } from '../../../utils/string_to_member_expression.js'; +import BindingWrapper from '../Element/Binding.js'; +import EventHandler from '../Element/EventHandler.js'; +import SlotTemplateWrapper from '../SlotTemplate.js'; +import Wrapper from '../shared/Wrapper.js'; +import bind_this from '../shared/bind_this.js'; +import is_dynamic from '../shared/is_dynamic.js'; import { is_head } from '../shared/is_head.js'; -import compiler_warnings from '../../../compiler_warnings.js'; -import { namespaces } from '../../../../utils/namespaces.js'; -import { extract_ignores_above_node } from '../../../../utils/extract_svelte_ignore.js'; +import mark_each_block_bindings from '../shared/mark_each_block_bindings.js'; const regex_invalid_variable_identifier_characters = /[^a-zA-Z_$]/g; @@ -77,11 +76,6 @@ export default class InlineComponentWrapper extends Wrapper { ).toLowerCase() }; if (this.node.children.length) { - this.node.lets.forEach((l) => { - extract_names(l.value || l.name).forEach((name) => { - renderer.add_to_context(name, true); - }); - }); this.children = this.node.children.map( (child) => new SlotTemplateWrapper( diff --git a/src/compiler/compile/render_dom/wrappers/SlotTemplate.js b/src/compiler/compile/render_dom/wrappers/SlotTemplate.js index 0d8b5f6b9c..855abfb8c8 100644 --- a/src/compiler/compile/render_dom/wrappers/SlotTemplate.js +++ b/src/compiler/compile/render_dom/wrappers/SlotTemplate.js @@ -38,10 +38,7 @@ export default class SlotTemplateWrapper extends Wrapper { type: 'slot' }); this.renderer.blocks.push(this.block); - const seen = new Set(lets.map((l) => l.name.name)); - this.parent.node.lets.forEach((l) => { - if (!seen.has(l.name.name)) lets.push(l); - }); + /** @type {import('./InlineComponent/index.js').default} */ (this.parent).set_slot( slot_template_name, get_slot_definition(this.block, scope, lets) diff --git a/src/compiler/compile/render_ssr/handlers/Slot.js b/src/compiler/compile/render_ssr/handlers/Slot.js index 4d1c38dc28..266bf006af 100644 --- a/src/compiler/compile/render_ssr/handlers/Slot.js +++ b/src/compiler/compile/render_ssr/handlers/Slot.js @@ -25,11 +25,6 @@ export default function (node, renderer, options) { : ${result} `); if (slot && nearest_inline_component) { - const lets = node.lets; - const seen = new Set(lets.map((l) => l.name.name)); - nearest_inline_component.lets.forEach((l) => { - if (!seen.has(l.name.name)) lets.push(l); - }); options.slot_scopes.set(slot, { input: get_slot_scope(node.lets), output: renderer.pop() diff --git a/src/compiler/compile/render_ssr/handlers/SlotTemplate.js b/src/compiler/compile/render_ssr/handlers/SlotTemplate.js index 8dd12a8004..14970eff02 100644 --- a/src/compiler/compile/render_ssr/handlers/SlotTemplate.js +++ b/src/compiler/compile/render_ssr/handlers/SlotTemplate.js @@ -20,11 +20,7 @@ export default function (node, renderer, options) { ); renderer.push(); renderer.render(children, options); - const lets = node.lets; - const seen = new Set(lets.map((l) => l.name.name)); - parent_inline_component.lets.forEach((l) => { - if (!seen.has(l.name.name)) lets.push(l); - }); + const slot_fragment_content = renderer.pop(); if (!is_empty_template_literal(slot_fragment_content)) { if (options.slot_scopes.has(node.slot_template_name)) { diff --git a/test/runtime/samples/component-slot-default-in-each/Nested.svelte b/test/runtime/samples/component-slot-default-in-each/Nested.svelte new file mode 100644 index 0000000000..d216843101 --- /dev/null +++ b/test/runtime/samples/component-slot-default-in-each/Nested.svelte @@ -0,0 +1,11 @@ + + +{#each {length: 3} as _, i} + +{/each} + +{#await promise then value} + +{/await} diff --git a/test/runtime/samples/component-slot-default-in-each/_config.js b/test/runtime/samples/component-slot-default-in-each/_config.js new file mode 100644 index 0000000000..5c0fd13871 --- /dev/null +++ b/test/runtime/samples/component-slot-default-in-each/_config.js @@ -0,0 +1,7 @@ +export default { + html: ` +
0 - undefined
+
1 - undefined
+
2 - undefined
+ ` +}; diff --git a/test/runtime/samples/component-slot-default-in-each/main.svelte b/test/runtime/samples/component-slot-default-in-each/main.svelte new file mode 100644 index 0000000000..2ff707e04d --- /dev/null +++ b/test/runtime/samples/component-slot-default-in-each/main.svelte @@ -0,0 +1,7 @@ + + + +
{item} - {value}
+
\ No newline at end of file diff --git a/test/runtime/samples/component-slot-let-scope-2/Nested.svelte b/test/runtime/samples/component-slot-let-scope-2/Nested.svelte new file mode 100644 index 0000000000..e231bf1a7f --- /dev/null +++ b/test/runtime/samples/component-slot-let-scope-2/Nested.svelte @@ -0,0 +1,3 @@ + + + \ No newline at end of file diff --git a/test/runtime/samples/component-slot-let-scope-2/_config.js b/test/runtime/samples/component-slot-let-scope-2/_config.js new file mode 100644 index 0000000000..6b2059f03d --- /dev/null +++ b/test/runtime/samples/component-slot-let-scope-2/_config.js @@ -0,0 +1,3 @@ +export default { + html: 'undefined2' +}; diff --git a/test/runtime/samples/component-slot-let-scope-2/main.svelte b/test/runtime/samples/component-slot-let-scope-2/main.svelte new file mode 100644 index 0000000000..8454c4acd1 --- /dev/null +++ b/test/runtime/samples/component-slot-let-scope-2/main.svelte @@ -0,0 +1,8 @@ + + + + {thing} + {thing} + \ No newline at end of file diff --git a/test/runtime/samples/component-slot-named-inherits-default-lets/Nested.svelte b/test/runtime/samples/component-slot-let-scope-3/Nested.svelte similarity index 82% rename from test/runtime/samples/component-slot-named-inherits-default-lets/Nested.svelte rename to test/runtime/samples/component-slot-let-scope-3/Nested.svelte index 472af6278e..cecce8db01 100644 --- a/test/runtime/samples/component-slot-named-inherits-default-lets/Nested.svelte +++ b/test/runtime/samples/component-slot-let-scope-3/Nested.svelte @@ -1,6 +1,4 @@ diff --git a/test/runtime/samples/component-slot-let-scope/Nested.svelte b/test/runtime/samples/component-slot-let-scope/Nested.svelte new file mode 100644 index 0000000000..772955e06e --- /dev/null +++ b/test/runtime/samples/component-slot-let-scope/Nested.svelte @@ -0,0 +1,2 @@ + + \ No newline at end of file diff --git a/test/runtime/samples/component-slot-let-scope/_config.js b/test/runtime/samples/component-slot-let-scope/_config.js new file mode 100644 index 0000000000..c0f4d29305 --- /dev/null +++ b/test/runtime/samples/component-slot-let-scope/_config.js @@ -0,0 +1,3 @@ +export default { + error: 'thing is not defined' +}; diff --git a/test/runtime/samples/component-slot-let-scope/main.svelte b/test/runtime/samples/component-slot-let-scope/main.svelte new file mode 100644 index 0000000000..e10a9a8584 --- /dev/null +++ b/test/runtime/samples/component-slot-let-scope/main.svelte @@ -0,0 +1,8 @@ + + + + {thing} + \ No newline at end of file diff --git a/test/runtime/samples/component-slot-nested-in-slot/One.svelte b/test/runtime/samples/component-slot-nested-in-slot/One.svelte index e27437c450..acb6a03358 100644 --- a/test/runtime/samples/component-slot-nested-in-slot/One.svelte +++ b/test/runtime/samples/component-slot-nested-in-slot/One.svelte @@ -3,6 +3,8 @@ export let a, b; - - + + + + diff --git a/test/runtime/samples/component-svelte-fragment-let-aliased/main.svelte b/test/runtime/samples/component-svelte-fragment-let-aliased/main.svelte index dabf1d65ef..5ca0cbbd10 100644 --- a/test/runtime/samples/component-svelte-fragment-let-aliased/main.svelte +++ b/test/runtime/samples/component-svelte-fragment-let-aliased/main.svelte @@ -4,8 +4,8 @@ export let things; - - + + {x} \ No newline at end of file diff --git a/test/runtime/samples/component-svelte-fragment-let-b/main.svelte b/test/runtime/samples/component-svelte-fragment-let-b/main.svelte index 584240d14d..bb24b97a3b 100644 --- a/test/runtime/samples/component-svelte-fragment-let-b/main.svelte +++ b/test/runtime/samples/component-svelte-fragment-let-b/main.svelte @@ -2,8 +2,8 @@ import Nested from './Nested.svelte'; - - + + {count} \ No newline at end of file diff --git a/test/runtime/samples/component-svelte-fragment-let-e/A.svelte b/test/runtime/samples/component-svelte-fragment-let-e/A.svelte index 3ba0e62cd9..f982aef7b1 100644 --- a/test/runtime/samples/component-svelte-fragment-let-e/A.svelte +++ b/test/runtime/samples/component-svelte-fragment-let-e/A.svelte @@ -3,8 +3,8 @@ export let x; - - + + {reflected} diff --git a/test/runtime/samples/component-svelte-fragment-let-e/_config.js b/test/runtime/samples/component-svelte-fragment-let-e/_config.js index 92c7028eae..1ececbb3ef 100644 --- a/test/runtime/samples/component-svelte-fragment-let-e/_config.js +++ b/test/runtime/samples/component-svelte-fragment-let-e/_config.js @@ -2,8 +2,6 @@ export default { html: ` 1 1 - 1 - 1 `, async test({ assert, target, component }) { @@ -14,8 +12,6 @@ export default { ` 2 2 - 2 - 2 ` ); } diff --git a/test/runtime/samples/component-svelte-fragment-let-e/main.svelte b/test/runtime/samples/component-svelte-fragment-let-e/main.svelte index 80c9a868cd..e95c8d9274 100644 --- a/test/runtime/samples/component-svelte-fragment-let-e/main.svelte +++ b/test/runtime/samples/component-svelte-fragment-let-e/main.svelte @@ -8,9 +8,3 @@ {reflected} - - - - {reflected} - - \ No newline at end of file diff --git a/test/validator/samples/lets-on-component/input.svelte b/test/validator/samples/lets-on-component/input.svelte new file mode 100644 index 0000000000..8ad7424245 --- /dev/null +++ b/test/validator/samples/lets-on-component/input.svelte @@ -0,0 +1,27 @@ + + + +

+ count in default slot: {count} +

+

+ count in bar slot: {count} +

+
+ + +

+ count in default slot: {count} +

+

+ count in bar slot: {count} +

+
+ + +

+ count in bar slot: {count} +

+
diff --git a/test/validator/samples/lets-on-component/warnings.json b/test/validator/samples/lets-on-component/warnings.json new file mode 100644 index 0000000000..9fb7827b47 --- /dev/null +++ b/test/validator/samples/lets-on-component/warnings.json @@ -0,0 +1,26 @@ +[ + { + "code": "missing-declaration", + "message": "let:count declared on parent component cannot be used inside named slot", + "start": { + "column": 22, + "line": 10 + }, + "end": { + "column": 27, + "line": 10 + } + }, + { + "code": "missing-declaration", + "message": "let:count declared on parent component cannot be used inside named slot", + "start": { + "column": 22, + "line": 25 + }, + "end": { + "column": 27, + "line": 25 + } + } +]