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.

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

Slotted elements missing in AST #6066

Closed
Conduitry opened this issue Mar 8, 2021 · 12 comments · Fixed by #6148
Closed

Slotted elements missing in AST #6066

Conduitry opened this issue Mar 8, 2021 · 12 comments · Fixed by #6148
Labels

Comments

@Conduitry
Copy link
Member

Describe the bug
The AST for components using a slotted element is missing the slotted element.

Logs
n/a

To Reproduce
Compile <Component><div slot='foo'></div></Component> with Svelte 3.35.0 and look at the AST.

Expected behavior
The AST should be as it was in 3.34.0 and include the <div>.

Stacktraces
n/a

Information about your Svelte project:
Svelte 3.35.0

Severity
The right JS code looks like it's still being generated, so this isn't huge. But it will affect tooling. Medium-high I'd say.

Additional context
This was brought to light by sveltejs/eslint-plugin-svelte3#94

@kkarpeev
Copy link

kkarpeev commented Mar 8, 2021

Hi! Is this somehow related to such compilation errors as below? Sometimes I get them on 6-7 components (mostly simple) that use slots. Couldn't reproduce it in REPL, unfortunately. ( Repeats unpredictably on 3.35.0 (only) during compilation.

ERROR in ./app/components/layout/Tabs.svelte
Module build failed (from ./node_modules/svelte-loader/index.js):
Error: TypeError: TypeError: renderer.add_string is not a function
    at C:\path_to_my_project\node_modules\svelte-loader\index.js:74:12

@Conduitry
Copy link
Member Author

@tanhauhau I'm looking at this now, and am a bit confused by what's going on. It looks like there's a new SlotTemplateWrapper for each child of an inline component, regardless of what's happening in the component there.

this.children = this.node.children.map(child => new SlotTemplateWrapper(renderer, block, this, child as SlotTemplate, strip_whitespace, next_sibling));

Is this intended? Did the AST change significantly with your changes for <svelte:fragment>? What I'm also confused about is that the AST that's output for something like <A><div/></A> still looks correct, with no SlotTemplate nodes.

@tanhauhau
Copy link
Member

oh no 😦

I rearranged the children of the InlineComponent node to wrap <element slot="xxx">, <Component slot="xxx"> with <svelte:fragment slot="xxx">, and all the remaining elements with <svelte:fragment>

In this step, I mutated the AST node. (L114)

const children = [];
for (let i=info.children.length - 1; i >= 0; i--) {
const child = info.children[i];
if (child.type === 'SlotTemplate') {
children.push(child);
info.children.splice(i, 1);
} else if ((child.type === 'Element' || child.type === 'InlineComponent' || child.type === 'Slot') && child.attributes.find(attribute => attribute.name === 'slot')) {
const slot_template = {
start: child.start,
end: child.end,
type: 'SlotTemplate',
name: 'svelte:fragment',
attributes: [],
children: [child]
};
// transfer attributes
for (let i=child.attributes.length - 1; i >= 0; i--) {
const attribute = child.attributes[i];
if (attribute.type === 'Let') {
slot_template.attributes.push(attribute);
child.attributes.splice(i, 1);
} else if (attribute.type === 'Attribute' && attribute.name === 'slot') {
slot_template.attributes.push(attribute);
}
}
children.push(slot_template);
info.children.splice(i, 1);
}
}
if (info.children.some(node => not_whitespace_text(node))) {
children.push({
start: info.start,
end: info.end,
type: 'SlotTemplate',
name: 'svelte:fragment',
attributes: [],
children: info.children
});
}

So, I have 2 options from here:

  1. used the updated AST with <svelte:fragment> inserted
+ // assign back the children with updated children
+ info.children = children
  this.children = map_children(component, this, this.scope, children);
  1. freeze the ast.html before mutation as well

// the instance JS gets mutated, so we park
// a copy here for later. TODO this feels gross
this.original_ast = {
html: ast.html,
css: ast.css,
instance: ast.instance && JSON.parse(JSON.stringify(ast.instance)),
module: ast.module
};

this.original_ast = {
	html: JSON.parse(JSON.stringify(ast.html)),
	css: ast.css,
	instance: ast.instance && JSON.parse(JSON.stringify(ast.instance)),
	module: ast.module
};

or rather

this.original_ast = JSON.parse(JSON.stringify({
	html: ast.html,
	css: ast.css,
	instance: ast.instance,
	module: ast.module
}));

@non25
Copy link

non25 commented Mar 11, 2021

Maybe it is better to involve community in testing this sort of changes in their production projects before pushing it to the master? Tell them what to look for or something.

Current way of getting to know something has regressed involves reading PR diffs and trying to speculate on that.

But that could be more transparently communicated from feature-pushers, saving time and making bug-squashing a more straightforward process. 🤔

FAQ assures that you shouldn't bother with testing svelte implementation details: https://svelte.dev/faq#how-do-i-test-svelte-apps

I think it would be better to live up to that expectation.

@non25
Copy link

non25 commented Mar 11, 2021

@Conduitry can you elaborate on what you found funny?

@pngwn
Copy link
Member

pngwn commented Mar 11, 2021

This problem is entirely internal. The AST is no specced and we make no guarantees of its stability. This has zero implication on production applications as it does not change the output of the compiler.

@non25
Copy link

non25 commented Mar 11, 2021

Hi! Is this somehow related to such compilation errors as below? Sometimes I get them on 6-7 components (mostly simple) that use slots. Couldn't reproduce it in REPL, unfortunately. ( Repeats unpredictably on 3.35.0 (only) during compilation.

ERROR in ./app/components/layout/Tabs.svelte
Module build failed (from ./node_modules/svelte-loader/index.js):
Error: TypeError: TypeError: renderer.add_string is not a function
    at C:\path_to_my_project\node_modules\svelte-loader\index.js:74:12

@pngwn I wouldn't write that if we didn't end up in a situation where 3.35 randomly throws compilation errors on components with slots.
Looks like something is trying to access a field of a null, and I've found related code in PR's diffs.
Do you have an idea how we should proceed to better isolate the issue?

@pngwn
Copy link
Member

pngwn commented Mar 11, 2021

Open a new issue with a simple reproduction.

@Conduitry
Copy link
Member Author

@tanhauhau I probably wouldn't be against cloning the whole AST. I do think I would want to try something like lukeed's https://github.com/lukeed/klona though, rather than JSON.parse(JSON.stringify(foo)) though. I think there are other places where we could benefit from its increased performance over stringify/parse as well. I'll take a look.

@Conduitry
Copy link
Member Author

One issue with the above suggestion of cloning the whole AST at that point for use later is that we'd fail the https://github.com/sveltejs/svelte/tree/master/test/parser/samples/textarea-children test, because we'd now be returning the <textarea> contents as children rather than normalizing them to be a value attribute. I'm not sure how important this is to have. It shouldn't affect the generated code, just the AST.

@dummdidumm
Copy link
Member

I want to point out that language tools (with svelte2tsx), the prettier plugin and the eslint plugin all rely on the AST to some extend, so while the API is technically private, it's still something we can't change it at will.

@sveltejs sveltejs deleted a comment from MrBigBanks Mar 12, 2021
@stalkerg
Copy link
Contributor

Base on this issue message it's can be only SSR render but base on user's reports they don't use SSR. It's strange.

Conduitry added a commit to Conduitry/svelte that referenced this issue Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants