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

Simplify script and meta tags #522

Closed
wants to merge 3 commits into from
Closed

Simplify script and meta tags #522

wants to merge 3 commits into from

Conversation

tkrotoff
Copy link
Contributor

@tkrotoff tkrotoff commented Dec 8, 2016 •
edited
Loading

Simplifications:

  • <script type="text/javascript" ...> replaced by <script ...>

See https://www.w3.org/TR/html5/scripting-1.html#attr-script-type “The default, which is used if the attribute is absent, is "text/javascript””

FYI The old HTML 4 specification requires type attribute but even in HTML 4 mode (<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">), IE6 works without specifying the type attribute (tested myself).

  • <meta http-equiv="Content-type" content="text/html; charset=utf-8"/> replaced by <meta charset="utf-8"/>

See https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Obsolete_things_to_avoid
See http://stackoverflow.com/questions/4696499

All tests passing locally.

Less code is always good :)

@tkrotoff
Remove unneeded script type attribute
4b5ee09 
See https://www.w3.org/TR/html5/scripting-1.html#attr-script-type “The default, which is used if the attribute is absent, is “text/javascript””
@tkrotoff
Simplify <meta> element and charset attribute
3c03d3a 
See https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Obsolete_things_to_avoid
See http://stackoverflow.com/questions/4696499
@tkrotoff
charset="UTF-8” => charset=“utf-8”
ee2df7c 
Lower case charset=“utf-8” is more common in the source base than
charset="UTF-8” (24 vs 6 occurrences), so let’s formalize to the lower case version
@tkrotoff tkrotoff mentioned this pull request Dec 8, 2016
Closed
@tkrotoff
Copy link
Contributor Author

tkrotoff commented Dec 8, 2016

No idea why tests fail with Travis CI, on my Mac npm run test runs just fine.

@numical
Copy link
Contributor

numical commented Dec 10, 2016

The type="text/javascript" has been explicitly added due to a Safari Bug and added with PR #311.

@tkrotoff
Copy link
Contributor Author

tkrotoff commented Dec 10, 2016 •
edited
Loading

@numical do you really believe that <script> without type does not work under Safari 9.1?? Half the web omits the type attribute, even http://www.apple.com/ does not use it.

The guy just did something wrong in his code, that's all.

@tkrotoff
Copy link
Contributor Author

tkrotoff commented Dec 10, 2016 •
edited
Loading

@numical LOL

So it turns out this wasn't entirely true. What was breaking my apps was [...]

=> no shit...

It is weird that this was even accepted

Btw, HTML minifiers remove type attribute anyway

Edit:
Nobody asked for a proof: small example exposing the bug or a link to a WebKit issue. And after accepting the PR, nobody filled a bug report in WebKit issue tracker. What is very nice from him is that he commented back to explain his mistake 👍

@numical
Copy link
Contributor

numical commented Dec 12, 2016

Just pointing out the history.
You will have seen @jantimon's reason for not backing out the PR, however mistaken, so I imagine he is unlikely to accept this one.

@tkrotoff
Copy link
Contributor Author

tkrotoff commented Dec 13, 2016 •
edited
Loading

Just pointing out the history

Thx for that. I genuinely made this PR thinking type was here by mistake. This PR also features <meta> tag simplification.

I imagine he is unlikely to accept this one

Maybe he will reconsider

@jantimon: [...] If your javascript is filtering for script files e.g. document.querySelector('script[type]') it would stop working

If someone wrote this - highly improbable IMHO - it will break sooner or later (HTML minifier, changing from html-webpack-plugin to another tool...).

The longer <script type="text/javascript" ...> stays, higher is the risk to see someone writing $('script[type]') e.g shooting himself through his foot.

If you are afraid someone might have written $('script[type]'), the solution is to bump semver major version number.

@SpaceK33z
Copy link
Contributor

Oh I just now see this PR. I made a PR removing only the type="text/javascript" thingies (#572) because this was discussed in #558. The difference is that my PR builds successfully though.

@tkrotoff
Copy link
Contributor Author

The difference is that my PR builds successfully though

This one too: Travis CI failed for other reasons.

@jantimon
Copy link
Owner

@SpaceK33z were merged into the version-3.x branch Thanks @tkrotoff @SpaceK33z

@jantimon jantimon closed this Jan 29, 2017
@tkrotoff
Copy link
Contributor Author

@jantimon There are 2 other commits in this PR:

that would be a nice addition

@jantimon
Copy link
Owner

Fixed thanks a lot 👍

jantimon added a commit that referenced this pull request Jan 29, 2017
@jantimon
Use charset="utf-8" as proposed in #522
4404f9d
@Daniel-Khodabakhsh
Copy link

Daniel-Khodabakhsh commented Mar 13, 2018 •
edited
Loading

I'm on 3.0.6 and html-webpack-plugin is still outputting <script type="text/javascript" ....
Looks like tag 3.0.6 doesn't come from the version-3.x branch.. which doesn't make sense to me.

@Daniel-Khodabakhsh Daniel-Khodabakhsh mentioned this pull request May 3, 2018
Closed
@lock
Copy link

lock bot commented May 31, 2018

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tkrotoff @numical @SpaceK33z @jantimon @Daniel-Khodabakhsh