fix(Static Template): Ensure valid HTML formatting for static template #894
Conversation
|
@joshwcomeau is attempting to deploy a commit to the CodeSandbox Team on Vercel. A member of the Team first needs to authorize it. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 92b797e:
|
|
|
||
| const domParser = new DOMParser(); | ||
| const doc = domParser.parseFromString(contentString, "text/html"); | ||
| doc.documentElement.setAttribute("lang", "en"); |
There was a problem hiding this comment.
So, I definitely don't think we should be forcing all Sandpacks to use English as their HTML language, but it occurred to me that right now, there's no way to set the lang attribute in a Sandbox that doesn't render an <html> tag. This is kind of an issue; for example, the CSS hyphens property only works when the lang attribute is set to "en" (and possibly other languages in the future).
Maybe this can be some sort of user-configurable setting?
There was a problem hiding this comment.
I think it's fine to default to en. Sandpack contains a lot of opinionated defaults, and we just need to provide a way to make it configurable, for example, persisting the html#lang value from the original document. Good insight, anyway!
There was a problem hiding this comment.
TIL DOMParser! Very useful API!
|
|
||
| return contentString; | ||
| console.timeEnd('domparser'); |
There was a problem hiding this comment.
I'll obviously remove the console.time stuff, but I left it in for now so y'all could see how I was diagnosing the times I mentioned in the PR :)
|
|
||
| return contentString; | ||
| return `<!DOCTYPE html>\n${html}`; |
There was a problem hiding this comment.
The documentElement.outerHTML property is a string that includes everything within the <html> tags, inclusively. But the <!DOCTYPE> sits outside the HTML tag. Therefore, we need to add it back in, even if the user included it in their content.
|
@danilowoz @DeMoorJasper Slightly unrelated question, but how might I use my fork of Sandpack within my course platform? I tried to point it to my Github fork in {
"dependencies": {
"@codesandbox/sandpack-react": "joshwcomeau/sandpack#52921c8",
}But this appears to have created the strong structure within |
|
@joshwcomeau, I was testing and taking this chance to add some unit tests on this PR. Feel free to merge or make any necessary changes.
Have you used CodeSandbox CI? For example, this PR creates a temp sandbox with the Sandpack packages, which includes all PR changes on a temporary registry. Check the following comment and the package.json inside of each sandbox #894 (comment) |
tests: add tests to `validateHtml` func
Thanks so much for the unit tests! I hope it wasn't too much trouble.
TIL about CodeSandbox CI! Will grab the csb.dev URL from that CodeSandbox and use it in my own repo to test these changes in my real-world project, thanks for the tip! |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
What kind of change does this pull request introduce?
I suppose it's a bug fix, though it's more about making things a bit more flexible!
What is the current behavior?
In #884, @SSHari added an automatic insertion. This works great if a full HTML document is passed, but it produces some funky results otherwise.
For example, if we have this input:
The
getFileContentmethod will prepend the DOCTYPE:+<!DOCTYPE html> <style> p { color: red; } </style> <p>Hello World!</p>…but when the browser actually renders the preview, that doctype is nowhere to be found.
I think the fundamental problem is that we're not giving it a proper HTML document. It's missing the
<html>,<head>, and<body>tags. The browser is remarkably good at taking a chunk of unformed HTML and producing a valid document (eg. the<style>gets added to a<head>, at least in Chrome), but it doesn't know how to handle<!DOCTYPE html>.What is the new behavior?
I'm using the DOMParser API to convert the provided HTML chunk into a valid HTML document. Then, I grab the text content of the
<html>tag, slap on the<!DOCTYPE>, and return it.What steps did you take to test this? This is required before we can merge, make sure to test the flow you've updated.
console.time. For most "realistic" scenarios, it takes about 1ms (using a 2021 MBP on 6x CPU throttle). I tried it with the full HTML of the Wikipedia homepage, and it took 14ms, but TBH with a document that big, each keystroke took ~500ms total, so the extra 14ms wasn't noticeable.I'd be happy to add unit tests, stories, etc. But first I wanted to see what y'all think of this approach.
Checklist