feat: Support setting <html> in _app template#1617
Merged
Conversation
marvinhagemeister
requested review from
bartlomieju,
hashrock,
iuioiua and
lucacasonato
marvinhagemeister
force-pushed
the
app-html
branch
from
7fbec72 to
e6932df
Compare
marvinhagemeister
force-pushed
the
app-html
branch
from
dd30423 to
4392630
Compare
bartlomieju
reviewed
Aug 11, 2023
bartlomieju
left a comment
bartlomieju
left a comment
Contributor
There was a problem hiding this comment.
From an initial review this looks good to me, but I must admit I'm not so knowledgable about this part. I would appreciate another pair of eyes for an approval.
fdemir
reviewed
Aug 11, 2023
| SLOTS_TRACKER.clear(); | ||
|
|
||
| // Clear rendering state | ||
| RENDERING_USER_TEMPLATE = false; |
Contributor
There was a problem hiding this comment.
RENDERING_USER_TEMPLATE should not be defined as screaming snake case. it confuses the reader, sounds constant.
Contributor
Author
There was a problem hiding this comment.
It's in uppercase to signal that it's a global variable in the context of the module and not a local one. I can rename it though. I'm not attached to the name
Contributor
Author
|
Merging, as I got verbal approval internally. |
Contributor
|
Looks great. Excited to be moving in the right direction! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This PR allows us to create the
<html>-tag ourselves in_app.tsx. In case we didn't render any<html>tag we fall back to the current way of adding an internal outer template. This allows you to set thelangattribute on<html>without having to go through various abstractions, etc.I tried various approaches to achieve this like adding a marker and replacing that in the HTML string. That proved to be bad for security as the marker could be injected by users. So I landed on the approach in this PR where we essentially swap out user supplied
<html>,<head>and<body>tags with an invisible Fragment. We store the props of the old vnode and after the body has rendered we'll render our internal outer template with the user supplied values. Basically this works close to how it worked before.Fixes #1529, fixes #1595