(fix) Fix the href not getting removed (#1943) #1965
Conversation
cristianbote
requested review from
developit,
marvinhagemeister and
JoviDeCroock
|
Look into this and basically https://github.com/preactjs/preact/pull/1965/files#diff-642a704212e8abfc3d854f17a1aaacbeR101 |
|
Is |
|
@marvinhagemeister Well that's kinda the issue here. Basically anything that I did tried that before and everything passed. But, is that the proper behaviour? Would it break custom elements 💯 support? 🤔 |
There was a problem hiding this comment.
I think this might be possible to fix elsewhere. Specifically, this line makes it so that diffing props where a key has an undefined value with props where that key doesn't exist will not call setProperty(), because oldProps[key] === newProps[key]:
Line 23 in 9ddc04e
I believe it may be possible to circumvent this by changing the comparison in the oldProps diff to consider undefined values of new props to be a "removal":
for (i in oldProps) {
- if (!(i in newProps)) {
+ if (newProps[i] === undefined) {
setProperty(dom, i, null, oldProps[i], isSvg);
}
}Also it looks like the 8.x version of DOM property diffing actually always removed any corresponding attribute for null values. I don't know that we should adopt that approach again, but it seems it may have been to work around this issue.
Line 113 in b54401f
|
This is breaking the uncontrolled input behaviour, which is true. Looking to fix that. |
The way MobX works is that every observed component returns false from shouldComponentUpdate unless props have changed. They than trigger the child renders themselves via direct forceUpdate calls. Our issue was that we only set the force flag sort-of globally during a render and would ignore any children that have been enqueued via forceUpdate in the same render cycle. With these changes we ensure that the force flag is always the one from the current component.
* Append portal node to container instead of prepend This fixes the issue described in #1806. Expected test outcomes were generated by taking the test code and running it with React instead of Preact. * Add additional test for portals testing ordering
|
I think this is ready. The added size is bothering me but oh well. Let me know what you think |
cristianbote
requested review from
marvinhagemeister,
JoviDeCroock and
developit
cristianbote
force-pushed
the
fix/skip-setter-for-href
branch
from
0c3c03a
to
9434a06
Compare
| dom[name] = value==null ? '' : value; | ||
| if (value==null) { | ||
| // This condition here guards against modifying a unonctrolled input's value | ||
| if (name!=='value' && name!=='checked') { |
There was a problem hiding this comment.
ooooh crap I hadn't thought of this implication. There are a bunch more names here that would have to be disabled, I think this might be a sign that we need to try different overall solutions...
There was a problem hiding this comment.
Yup, you're right. I'm gonna start looking at it from other angles
Co-Authored-By: Jason Miller <developit@users.noreply.github.com>
|
I'm gonna close this one since I'm pursuing another option here. Will open a different one. |
This fixes #1943
Adds+4B🙁Shaves-1B😁Adds+15B😢Adds
+12B😢