-
Notifications
You must be signed in to change notification settings - Fork 0
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
Getting rid of the svelte-retag wrapper #20
Comments
FWIW: Coincidentally, that particular code comment was in reference to the wrapping of the shadow DOM, which was added here and was a bug fix, since I was addressing layout issues induced by the That said, I do agree that the use of Unfortunately, as I started to write it out, it started to turn into a pretty massive thesis, so I'll just give short bullet points. I'll save what I wrote for a post later when I hopefully have time to fill it out a bit more. Why the
|
Note: Just adding Just note that some of the older unit test code ( Thankfully unit testing is easy to setup, just clone down and set it up:
|
Thank you for sharing your thoughts and background information! I'd like to contribute more, but I have more of a 'rudimentary developer' skill set. |
No worries at all. You’re not the only one to notice and ask about it. It appears to not affect the performance at all, but it is an interesting puzzle (getting the end result we want while still having all the features we need). |
Lumping this into v2 since technically this could be a breaking change if anyone has unit tests (or anything else) that somehow depends on the presence of the |
Hmm... sorry folks, still figuring this out! 😅 I need some way to classify something as a breaking change (and thus must be in a future version) but also want a clear way of communicating what will definitely make it into the next version (i.e. sort of like approved for v2 and v3, etc). So for now it cannot be v1 but I'm not sure it'll make it into v2 either, so I'll create a new "one day" milestone instead. |
@Vanillabacke it's been a minute since I've gotten back on this with any real detail. That said, I've taken some time today to really dive into this myself and I can provide a bit more feedback. At least on the biggest use case for the Essentially, that wrapper is central to solving several complicated problems all together in combination:
Technical breakdownEach has some kind of tradeoff that factors into the other in some form. The crux of the issue boils down to how browsers perform DOM rendering. On initial page load, the browser will piece together the DOM from the top down. During this phase (i.e. However: During this So: Since we don't want to miss too many frames and create any "jank" as the page is loading, we try to render the component somewhere. The Speed ⚡The best speed is with Performance 🚀Lots of things could potentially trigger a re-render (e.g. DOM appending new child nodes directly under your tag), so Overlap ⚡ + 🚀There's some overlap of course; Alternatives consideredI've been able to accomplish all of the above without the
ConclusionTo me, at least for light DOM (using I'll also continue to do more research on ways to balance the best of all points noted above with More readingThis is a common frustration. See:
|
Note: One important gradual step in this direction is tweak the older unit tests that depend on the presence of the |
Describe the problem
Using the customElement as a wrapper as itself instead of the ....
I saw your comment:
// TODO: Better than <div>, but: Is a wrapper entirely necessary? Why not just set this._root = this.shadowRoot?
So I guess you should know, what I mean:
and a comp.svelte:
it will render as:
Describe the proposed solution
Havn't tested it fully, but for me it is working with
shadow: false
:and in the
_renderSvelteComponent()
on line 463:Alternatives considered
would be nice to reduce the the nested tags, but it's just a nice to have. tried to figure out the tests, but I'm not using them yet and got a lot of expection / received errors of the changed render output.
Importance
nice to have
Note from @patricknelson
Address outstanding
TODO: ISSUE-20
items.The text was updated successfully, but these errors were encountered: