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

3128: Test to show nested slots fails #3136

Closed

Conversation

cam-stitt
Copy link

To go along with my investigations in #3128, I have added a custom element test for a nested custom element that shows the failure I am receiving. The slot is never assigned to the nested element.

Note: Apologies for the console event change, it was the only way I could get test logs to work.

@cam-stitt
Copy link
Author

I now have a change that allows the test to pass. I'm not confident that I have made the right changes with regards to $$.slotted, but this does work. I've also tested it with cam-stitt/svelte-nested-ce and everything worked as expected.

@pbastowski
Copy link

pbastowski commented Aug 18, 2019

I have just tested your PR with my test project and slots now do indeed work as expected.

Here is a link to my test repo: https://github.com/pbastowski/svelte-nested-slot-inside-web-component-fix-test

That is:

  1. The slot content from index.html is passed into App.svelte and renders correctly within it's <slot />.

  2. App.svelte imports and uses Input.svelte, which exposes a slot, and that slot's contents are rendering as expected.

Without this PR the slot within Input.svelte did not render at all.

Based on my limited testing, it would appear that this PR has indeed fixed the broken slot problem.

@pbastowski
Copy link

Update: this PR breaks sveltestrap

I added the sveltestrap component library to the project and tried to use it's components in the project. However, it doesn't work with this PR, but works with the latest stable version of Svelte.

How does it break?

Well, the template content starting with and below a sveltestrap Button does not render and the dev console displays this error:

Uncaught (in promise) TypeError: Failed to execute 'insertBefore' on 'Node': parameter 1 is not of type 'Node'.

I have added branch use-sveltestrap to my project to show this issue.

@campbeln
Copy link

campbeln commented Feb 5, 2020

Uncaught (in promise) TypeError: Failed to execute 'insertBefore' on 'Node': parameter 1 is not of type 'Node'.

Sounds like we just need to ensure the argument to insertBefore (likely at the @insert(${ parent_node || '#target'}, ${name}, ${parent_node ? 'null' : 'anchor'}); call?) is a Node:

function isNode(x) {
    return (x && x.nodeName && x.nodeType > 0 && x.nodeType < 13);
}

(ugly hacky test based on https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType but it gets the idea across).

@campbeln
Copy link

campbeln commented Feb 6, 2020

I've updated the most recent version of Svelte with the changes per 0c397ae (sans the /examples/ changes as those file don't seem to be present any longer) here: https://github.com/campbeln/svelte

Unfortunately... these changes don't seem to fix the issues any longer.

@campbeln
Copy link

campbeln commented Feb 13, 2020

Fix for current build is contained in master...campbeln:master and an example of working code is included in #4402.

NOTE: Not certain if change to line 23 of src/compiler/compile/render_dom/index.ts is necessary. Also not certain if this also breaks sveltestrap as noted above - #3136 (comment) .

@Conduitry Conduitry marked this pull request as draft June 3, 2020 21:10
@stale
Copy link

stale bot commented Jun 26, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Jun 26, 2021
@stale stale bot removed the stale-bot label Jun 26, 2021
@stale stale bot removed the stale-bot label Jun 27, 2021
@benmccann
Copy link
Member

Thanks for sharing this. I'm going to go ahead and close it since it can't be merged, but it should show up on the referenced ticket so that it can be leveraged if someone has a fix for the issue

@benmccann benmccann closed this Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants