more comprehensive property/attribute setting #14
Conversation
|
Still a WIP |
jquense
changed the title
|
I did a bunch of work on the prop/attribute stuff and got some SVG support as well. I thought about gating the SVG the support behind a flag so folks that don't need it could be spared the extra weight, but it's not a ton (mostly annoying property configs) so maybe not worth it |
webpack.config.js
Outdated
| plugins.extractText(), | ||
| plugins.uglify({ parallel: 4 }), |
There was a problem hiding this comment.
oops, just a quick test of the DCE
There was a problem hiding this comment.
Sidenote: I have always wondered why it takes three tools: uglify rollup and GCC for DCE.
There was a problem hiding this comment.
i guess, GCC probably replaces ugilfy in this case?
There was a problem hiding this comment.
I guess. And rollup does a bit of elimination too. And I'm not talking about tree shaking of ES6. I have seen
if (true) {
// code
}
being turned to
{
// code
}
by rollup
There was a problem hiding this comment.
You don’t need Uglify if you use GCC. In fact adding Uglify can make things worse if you have GCC-d code
| ): boolean { | ||
| DOMComponent.setInitialProps(domElement, props); | ||
| if (isSvg == null) isSvg = getRootSvgContext(rootContainerInstance); |
There was a problem hiding this comment.
this feels like it should unnecessary...I wonder if react-reconciler could pass the HostContext here?
We need the context here because unlike react-dom, which has an "attributes first" approach, rdl is "property first" which also means we need to intentionally skip that path for SVGs, hence need to know if we are in an SVG context or not.
@gaearon I took a look at the reconciler code but it wasn't clear (to me) whether passing HostContext is possible at this stage of the commit. Do you have a sense of that?
There was a problem hiding this comment.
I think we don’t have host context during the commit phase.
During reconciliation, we go through every host component so we can track it.
During commit, we only go through the fibers marked as having “effects”. It’s a much shorter linked list of things that actually need DOM/lifecycle updates.
So we can’t have the current host context value during the commit because we jump over many nodes.
There was a problem hiding this comment.
Wait, I misunderstood. finalizeInitialChildren is not in the commit phase.
Only things called from CommitWork are commit phase. Things called from BeginWork or CompleteWork are reconciliation phase.
So yes, I think you could pass host context there if you want to. (If the reconciler doesn’t you can send a PR)
I think this idea has been floated here before My worry is this would add configuration to the build step. |
|
lets just leave off the feature toggling for now. the config doesn't add that much extra, and things like preact already include svg support by default (admittedly it's easier for them because they don't have to normalize prop names) |
|
gtg? |
|
@jquense I don't have any more feedback here! I'd be to be a sounding board if you want to flesh out some ideas. Edit: Also sorry to get to this so late |
change -> input svg and fix properties
cc @nhunzaker