Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Inject HTML tag if missing #654

Merged
merged 2 commits into from
Jan 29, 2018
Merged

Inject HTML tag if missing #654

merged 2 commits into from
Jan 29, 2018

Conversation

brandon93s
Copy link
Contributor

Fixes #633 if we want to support this case.

@codecov-io
Copy link

Codecov Report

Merging #654 into master will decrease coverage by 0.45%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #654      +/-   ##
==========================================
- Coverage   89.02%   88.57%   -0.46%     
==========================================
  Files          61       61              
  Lines        1896     1900       +4     
==========================================
- Hits         1688     1683       -5     
- Misses        208      217       +9
Impacted Files Coverage Δ
src/packagers/HTMLPackager.js 94.11% <60%> (-5.89%) ⬇️
src/assets/CSSAsset.js 82.35% <0%> (-7.36%) ⬇️
src/Logger.js 32.72% <0%> (-5.46%) ⬇️
src/utils/installPackage.js 93.75% <0%> (+6.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9596d6...bfa3c15. Read the comment docs.

@farskid
Copy link
Contributor

farskid commented Jan 25, 2018 •
edited
Loading

So basically this would inject the html tag if not found? if so, is it possible to check for body and head too?

@brandon93s
Copy link
Contributor Author

brandon93s commented Jan 25, 2018 •
edited
Loading

@farskid - That's correct.

Parcel already handles injecting the head for you if it isn't found. The body would also be possible. The body isn't required by the html5 spec though; it's left as an implementation detail for the browser to handle it.

@devongovett
Copy link
Member

We should probably handle it. Browsers do, so HTML like this exists in the wild.

Question: in the example from the issue, there is no <html>, <head>, or <body>. In this PR, Parcel will add the <html>, and a <head> tag inside it. But content that's already in there like the <link> tag won't get moved into the <head> and the other stuff won't get moved into a <body>. It's unclear what actually happens to that stuff.

Perhaps we should actually fix it as browsers do? There is probably a spec for this to determine how to insert the elements. Otherwise, since the tags are optional, we could just insert the element into the root content and not even bother adding an <html> or <head> tag.

@devongovett
Copy link
Member

For reference: https://www.w3.org/TR/2011/WD-html5-20110525/syntax.html#optional-tags

@farskid
Copy link
Contributor

farskid commented Jan 25, 2018

@brandon93s @devongovett Good point. in addition to what you said, in the example on issue, if I added the head tag even without html tag, parcel would bundle successfully. isn't it against the spec too?

@farskid farskid mentioned this pull request Jan 25, 2018
Closed
@brandon93s
Copy link
Contributor Author

After some research and digging into WebKit, it looks like the html, head and body are all optional in an HTML5 document. I'm of the opinion that parcel shouldn't unnecessarily alter the user's html but instead insert the bundle tags in a location that produces explainable and consistent output.

Browsers will automatically break up your html into body and head tags using some relatively straightforward logic. The gist of it is: if no head is found, it scans nodes until it finds the first node that is not allowed to be in the head. Once it encounters that node, everything above it is placed into a header and the rest is candidate for the body. We can use this information to place the bundles in the tree where we know they'll end up in the head, without having to manually add the head, html or body to the users document.

Adding html, body and head could also have unintended side-effects down the line if an html-partial were to pass through here.

With the above in mind, I propose the following strategy:

diagram 1

This would remove the current code that adds the head node, and implements additional logic to find the correct position to insert the bundle tag if no head is found.

Thoughts? Will take a shot at implementation pending feedback. @devongovett

@devongovett
Copy link
Member

Yeah that sounds like the right strategy.

@brandon93s
Copy link
Contributor Author

The above discussed strategy has been implemented.

devongovett
devongovett approved these changes Jan 29, 2018
View reviewed changes
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! ✨

@devongovett devongovett merged commit 5a37322 into parcel-bundler:master Jan 29, 2018
@devongovett devongovett mentioned this pull request Jan 31, 2018
Closed
@brandon93s brandon93s deleted the wrap-html branch February 18, 2018 18:55
devongovett pushed a commit that referenced this pull request Oct 15, 2018
@brandon93s @devongovett
Inject HTML tag if missing (#654)
6c0ba48 
* Inject HTML tag if missing

* Update sibling bundle node insert logic
devongovett pushed a commit that referenced this pull request Oct 15, 2018
@brandon93s @devongovett
Inject HTML tag if missing (#654)
7434553 
* Inject HTML tag if missing

* Update sibling bundle node insert logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devongovett devongovett devongovett approved these changes

Assignees
No one assigned
Labels
🐛 Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brandon93s @codecov-io @farskid @devongovett