fixing async loader for v3 #952
Conversation
prateekbh
requested review from
developit and
ForsakenHarmony
and removed request for
developit
| @@ -48,7 +48,11 @@ if (typeof app === 'function') { | |||
| * to send other data like at some point in time. | |||
| */ | |||
| const CLI_DATA = { preRenderData }; | |||
| root = render(h(app, { CLI_DATA }), document.body, root); | |||
| const doRender = | |||
| process.env.NODE_ENV !== 'production' || root.tagName !== 'script' | |||
There was a problem hiding this comment.
wondering if we need to recommend folks use id="app" here. We could have L35 do:
let root = document.getElementById('preact_root') || document.body.firstElementChild;Thoughts? It would help us avoid browser extensions potentially changing the value of firstElementChild on us.
There was a problem hiding this comment.
yes! I really really want to do this. This stream lines everything for us
There was a problem hiding this comment.
totally agree with it. :) Always wanted to do this one
|
Looks perfect aside from the one question about assuming firstElementChild. I think we can move forward with this regardless. @prateekbh Only other thing would be if we want to consider having something for Preact 8? Or are we officially dropping support for it as of this PR? |
I guess lets keep it simple |
|
@prateekbh right, just currently CLI V3 supports Preact 8 and X at the same time. |
|
Does this handle route transitions as well now? |
|
Ahh good point. This needs to only throw during hydration! |
TO BE MERGED after: preactjs/preact#2259
What kind of change does this PR introduce?
Bug fix
Did you add tests for your changes?
No
Summary
Does this PR introduce a breaking change?
No