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

fix: detach error when node is not attached to DOM #3156

Closed
wants to merge 1 commit into from
Closed

fix: detach error when node is not attached to DOM #3156

wants to merge 1 commit into from

Conversation

btakita
Copy link
Contributor

@btakita btakita commented Jul 2, 2019 •
edited
Loading

@btakita btakita changed the title fix https://github.com/sveltejs/svelte/issues/2086: detach error when… fix: detach error when node is not attached to DOM Jul 2, 2019
@Rich-Harris
Copy link
Member

Thanks. I think this is working around the issue rather than solving it though — I think it's better to identify the root causes. In this case, it seems that <head> children can get detached twice, because there's no if (detaching) guard around them. I've opened a tentative PR, #3159 — will close this in favour of that

@Rich-Harris Rich-Harris closed this Jul 2, 2019
@btakita
Copy link
Contributor Author

btakita commented Jul 2, 2019 •
edited
Loading

@Rich-Harris I agree that it's better to find the root cause. I think this guard is important in addition because this same exception has re-occurred whenever there has been a race condition or issue with detaching nodes.

As discussed in the #updates discord channel, I didn't add a warning because I was not sure if the upstream cause of this error is a legitimate issue so I opted to keep it simple. Should there be a warning if node.parentNode is falsy?

@btakita
Copy link
Contributor Author

btakita commented Jul 4, 2019 •
edited
Loading

@Rich-Harris There's a few more reasons to use the conditional in detach.

  1. The Svelte compiler cannot guarantee that this condition will not occur. If another library removes the DOM element before destroy is called, TypeError: Cannot read property 'removeChild' of null will occur. While the programmer is not supposed to interact with the DOM while using Svelte, other libraries that interact with the DOM will be used.
  2. This error case (TypeError: Cannot read property 'removeChild' of null) results in a blank screen. It's better to make Svelte/Sapper more robust than rendering a blank screen.
  3. Having the conditional in detach is effectively a generalized conditional for if (detach) block.d(1);, reducing the need to have these conditionals/concerns spread over the codebase.
  4. This class of bugs/errors is difficult to reproduce.

This was referenced Jul 5, 2019
Closed
Closed
Closed
Merged
@Conduitry
Copy link
Member

I guess it makes sense to have the conditional in detach. I mean ideally, we wouldn't need that check, and I also don't know how far we want to go down the road of 'be able to handle it when other code messes with the DOM'. I'm also worried about doing this masking real bugs in Svelte itself.

Perhaps as a compromise, we can log something scary sounding ('if you aren't messing with the DOM in some other way, this shouldn't happen', except scarier) if there's no parent node and we're compiling in dev mode?

@pngwn
Copy link
Member

pngwn commented Jul 7, 2019

Svelte is already pretty forgiving when it comes to using third-party libraries that interact with the DOM, i don't know if it makes sense to cater to them directly.

@btakita
Copy link
Contributor Author

btakita commented Jul 7, 2019 •
edited
Loading

I'm not asking svelte to spend time catering to any library, though it's a bonus if Svelte is interoperable with other libraries. As somebody who uses Svelte, I've run into this bug before (internal to Svelte & not caused by outside code) & it resulted in a blank screen & I was unable to reproduce the issue in the repl. It's also difficult to reason about, the stack trace does not lead to the source of the problem (so a binary search of commenting out component logic is necessary to locate the offending code), & leaves the programmer with less confidence in using Svelte.

This issue will come up again & rather than wasting everybody's time with the bug report, reproduction, discussion, we should remove the possibility of it appearing again.

This fix even saves code. Rather than generating n if (detach) conditionals in the output, we would have only 1 if (node.parentNode).

@btakita
Copy link
Contributor Author

btakita commented Jul 7, 2019 •
edited
Loading

@Conduitry I see your point re: this exception (or warning) providing a cue that something occurred that should not have occurred. I just wonder if one of the core purposes of Svelte is to ensure that detach is called only once per DOM node. I don't think this is a problem that a programmer using Svelte cares about. It has negligible performance implications & does not seem to affect any of the component destroy logic. I understand that unnecessary calls to detach may indicate that optimization needs to occur, though there's probably other ways of determining if unnecessary calls are being made.

To your point, the warning could occur if (!node.parentNode). That would at least remove the blank screen due to the exception. It would still cause a bug report & time to reproduce, which I suspect will yield false positives re: the function of Svelte itself; Meaning the only issue will be without any other problem related to the operation of the software:

"You or another library should not manually remove a DOM element"
"Why not?"
"You just shouldn't"

Other than detach being called on a DOM element already removed, what is the actual problem? If detach being called on a DOM element already removed is not considered a problem, would there be any other problem?

@btakita
Copy link
Contributor Author

btakita commented Jul 7, 2019

To prevent further forking of this discussion, I'd like to point out that it's been moved to #3172

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@btakita @Rich-Harris @Conduitry @pngwn