Bounds must be non-empty
#856
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chancancode
force-pushed
the
fix-bounds
branch
2 times, most recently
from
3a96c77
to
84c6638
Compare
* Inline `insertHTMLBefore` helper and stop exporting it (technically breaking) * Align types in signatures * Unify the handling of empty content
chancancode
force-pushed
the
fix-bounds
branch
from
84c6638
to
01231a8
Compare
chancancode
commented
Sep 27, 2018
| let first = prev ? prev.nextSibling : parent.firstChild; | ||
| let last = reference ? reference.previousSibling : parent.lastChild; | ||
|
|
||
| return new ConcreteBounds(parent, first, last); |
There was a problem hiding this comment.
This is a bug (similar in nature to #852) in FastBoot. Since you don't normally run updates there I don't know how much this matters (i.e. should I split this into a BUGFIX PR and release that separately).
chancancode
force-pushed
the
fix-bounds
branch
from
6118f18
to
afab947
Compare
The `Bounds` object represents a _inclusive_ range. That is, the bounds is referring to the range in DOM _including_ the `firstNode()` and the `lastNode()`. Therefore, it does not ever make sense for those endpoints to be `null`. (Technically, `(parent, null, null)` is a valid way to reprsent an _empty_ range. However, in practice we do not have a use case for this, and every use of `null` were actual bugs.) This is technically a breaking change, but then again, if `null` were passed today it's likely a bug, and if the consuming side was checking for `null` it's likely just deadcode that will never execute in practice.
chancancode
force-pushed
the
fix-bounds
branch
from
afab947
to
32811df
Compare
chancancode
commented
Sep 28, 2018
| if (parent.namespaceURI !== svgNamespace) { | ||
| return super.insertHTMLBefore(parent, nextSibling, html); | ||
| } | ||
|
|
||
| return fixSVG(parent, div, html, nextSibling); | ||
| // TODO: why are these casts okay??? |
There was a problem hiding this comment.
There was a problem hiding this comment.
The mental model is that DOMChanges is dealing with live elements. Updating doesn't mean anything in SSR case.
chancancode
commented
Sep 28, 2018
| return new ConcreteBounds(_parent, comment, comment); | ||
| } | ||
|
|
||
| // TODO why are these casts okay??? |
There was a problem hiding this comment.
rwjblue
approved these changes
Sep 28, 2018
|
I don’t think so. I also removed some exports too, but I don’t know of any consumers |
|
On the other hand, it doesn’t cost anything to bump the minor |
This commit finishes the job iof making the SimpleDOM abstraction the full abstraction for DOM implementations in Glimmer. It adds a subset of the standard insertAdjacentHTML API to SimpleDOM, which is expected to be present in all DOM implementations provided to Glimmer.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
💣 contains breaking changes
The
Boundsobject represents a inclusive range. That is, the bounds is referring to the range in DOM including thefirstNode()and thelastNode(). Therefore, it does not ever make sense for those endpoints to benull. (Technically,(parent, null, null)is a valid way to represent an empty range. However, in practice we do not have a use case for this, and every use ofnullwere actual bugs.)This is technically a breaking change, but then again, if
nullwere passed today it's likely a bug, and if the consuming side was checking fornullit's likely just deadcode that will never execute in practice.