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.

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

Manipulation: Add support script type module #3869

Closed
wants to merge 7 commits into
base: master
from
Copy path View file
@@ -3,12 +3,19 @@ define( [
], function( document ) {
"use strict";
function DOMEval( code, doc ) {
function DOMEval( code, doc, type, src ) {

This comment has been minimized.

@gibson042

gibson042 Dec 4, 2017

Member

See #3766. I think this is sufficient cause to pass around the node object itself, instead of adding parameter after parameter.

This comment has been minimized.

@tbepdb

tbepdb Dec 4, 2017

Contributor

function globalEval depends from DOMEval, probably we should write another function something like DOMScript, that function can receive node object. also this function can include all checks like

	if ( rscriptType.test( node.type || "" ) &&
		!dataPriv.access( node, "globalEval" ) &&
		jQuery.contains( doc, node ) ) {

		if ( node.src && node.type !== "module" ) {

This comment has been minimized.

@gibson042

gibson042 Dec 4, 2017

Member

DOMEval is already an internal function, and only invoked at two preexisting sites. We don't need to introduce another function, we just need to update the signature to e.g. function DOMEval( code, doc, node ) and copy over whitelisted attributes (src, type, crossorigin, integrity, etc.) in the body.

doc = doc || document;
var script = doc.createElement( "script" );
script.text = code;
if ( code ) {

This comment has been minimized.

@gibson042

gibson042 Dec 4, 2017

Member

I don't think we need this condition.

script.text = code;
}
if ( type === "module" ) {

This comment has been minimized.

@gibson042

gibson042 Dec 4, 2017

Member

The HTML Standard specifies a case-insensitive match for "module", which we need to respect.

script.type = type;
}
if ( src ) {
script.src = src;
}
doc.head.appendChild( script ).parentNode.removeChild( script );
}
Copy path View file
@@ -193,14 +193,19 @@ function domManip( collection, args, callback, ignored ) {
!dataPriv.access( node, "globalEval" ) &&
jQuery.contains( doc, node ) ) {
if ( node.src ) {
if ( node.src && node.type !== "module" ) {

This comment has been minimized.

@gibson042

gibson042 Dec 4, 2017

Member

It's very convenient that module execution is inherently asynchronous with respect to parsing. But I suppose we'll also need awareness of nomodule now.

// Optional AJAX dependency, but won't run scripts if not present
if ( jQuery._evalUrl ) {
jQuery._evalUrl( node.src );
}
} else {
DOMEval( node.textContent.replace( rcleanScript, "" ), doc );
DOMEval(
node.textContent.replace( rcleanScript, "" ),
doc,
node.type,
node.src
);
}
}
}
@@ -1,5 +1,5 @@
define( function() {
"use strict";
return ( /^$|\/(?:java|ecma)script/i );
return ( /^$|module|\/(?:java|ecma)script/i );

This comment has been minimized.

@gibson042

gibson042 Dec 4, 2017

Member

^module$

} );
Copy path View file
@@ -1757,11 +1757,13 @@ function testHtml( valueObj, assert ) {
"<script type='something/else'>ok( false, 'evaluated: non-script' );</script>",
"<script type='text/javascript'>ok( true, 'evaluated: text/javascript' );</script>",
"<script type='text/ecmascript'>ok( true, 'evaluated: text/ecmascript' );</script>",
"<script type='module'>ok( true, 'evaluated: module' );</script>",
"<script>ok( true, 'evaluated: no type' );</script>",
"<div>",
"<script type='something/else'>ok( false, 'evaluated: inner non-script' );</script>",
"<script type='text/javascript'>ok( true, 'evaluated: inner text/javascript' );</script>",
"<script type='text/ecmascript'>ok( true, 'evaluated: inner text/ecmascript' );</script>",
"<script type='module'>ok( true, 'evaluated: inner module' );</script>",

This comment has been minimized.

@gibson042

gibson042 Dec 4, 2017

Member

We should also have a test for a module script with src.

This comment has been minimized.

@mgol

mgol Dec 4, 2017

Member

This won't work in browsers not supporting modules, will it? A separate iframe test might be better, especially that these tests are already too big.

"<script>ok( true, 'evaluated: inner no type' );</script>",
"</div>"
].join( "" ) )
ProTip! Use n and p to navigate between commits in a pull request.