Manipulation: Add support script type module #3869

tbepdb · 2017-12-03T10:34:21Z

Summary

For method
$('selector').html()
append support native ECMAScript modules, like that
<script type="module" scr="PATH/file.js"></script>
and like that

<script type="module">
  import * as myModule from "PATH/myModule.js";
</script>

Checklist

New tests have been added to show the fix or feature works
All authors have signed the CLA at https://cla.js.foundation/jquery/jquery
Grunt build and unit tests pass locally with these changes
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com

gibson042

Thanks for the pull request. Can you also reference a relevant issue, creating one if necessary?

gibson042 · 2017-12-04T02:56:58Z

src/manipulation/var/rscriptType.js

@@ -1,5 +1,5 @@
 define( function() {
 	"use strict";

-	return ( /^$|\/(?:java|ecma)script/i );
+	return ( /^$|module|\/(?:java|ecma)script/i );


gibson042 · 2017-12-04T03:15:54Z

src/manipulation.js

@@ -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" ) {


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.

gibson042 · 2017-12-04T03:19:51Z

src/core/DOMEval.js

@@ -3,12 +3,19 @@ define( [
 ], function( document ) {
 	"use strict";

-	function DOMEval( code, doc ) {
+	function DOMEval( code, doc, type, src ) {


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

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" ) {

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.

gibson042 · 2017-12-04T03:46:51Z

src/core/DOMEval.js

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


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

gibson042 · 2017-12-04T18:45:18Z

src/core/DOMEval.js

 		doc = doc || document;

 		var script = doc.createElement( "script" );
-
-		script.text = code;
+		if ( code ) {


I don't think we need this condition.

gibson042 · 2017-12-04T18:51:17Z

src/core/DOMEval.js

+		}
+		jQuery.each( [
+			"type",
+			"nomodule",


jQuery.each is almost certainly overkill for what should be for ( property in scriptProperties ) iteration when node is provided, and "nomodule" doesn't belong in the list because it is ignored on module scripts (which are the only cases where we have a node here).

gibson042 · 2017-12-04T18:54:48Z

src/manipulation.js

@@ -193,14 +193,14 @@ function domManip( collection, args, callback, ignored ) {
 						!dataPriv.access( node, "globalEval" ) &&
 						jQuery.contains( doc, node ) ) {

-						if ( node.src ) {
+						if ( node.src && !/^module$/i.test( node.type ) ) {


We don't like inlining regex definitions; this should be node.type.toLowerCase() === "module" unless a supported browser has non-string type properties, in which case the regex should be defined outside the function.

gibson042 · 2017-12-04T18:55:16Z

test/unit/manipulation.js

 			"<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>",


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

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.

timmywil · 2017-12-11T17:08:36Z

I think we would like to separate the module tests from other tests and only run them if modules are supported.

gibson042 · 2017-12-11T17:08:43Z

src/core/DOMEval.js

 		script.text = code;
+		if ( node ) {
+			for ( i in { type: true, src: true } ) {


This object should be a preservedScriptAttributes variable, not a literal buried in the function body.

tbepdb · 2017-12-11T17:26:04Z

Browser is skipping scripts by default in case type is unsuported. this means tests wont run in case modules arent supported.

timmywil · 2017-12-11T17:41:49Z

@tbepdb still, we'd like to separate all module-related tests from the others.

…d tests.

tbepdb added 2 commits December 3, 2017 15:58

Manipulation: Add support script type module

24e659b

Manipulation: Add support script type module

a8bbd23

gibson042 requested changes Dec 4, 2017

View reviewed changes

Manipulation: Add support script type module. Review fixes.

23ad984

gibson042 requested changes Dec 4, 2017

View reviewed changes

tbepdb added 2 commits December 5, 2017 01:23

Manipulation: Add support script type module. Review fixes.

4bf3732

Manipulation: Add support script type module. Add checks for undefined.

3880d89

gibson042 requested changes Dec 11, 2017

View reviewed changes

Manipulation: Add support script type module. Review fixes.

c357ab4

Manipulation: Add support script type module. Separated module-relate…

2926099

…d tests.

timmywil closed this in 5d3a968 Jan 16, 2018

lock bot locked as resolved and limited conversation to collaborators Jul 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manipulation: Add support script type module #3869

Manipulation: Add support script type module #3869

tbepdb commented Dec 3, 2017 •

edited by timmywil

gibson042 left a comment

gibson042 Dec 4, 2017

gibson042 Dec 4, 2017

gibson042 Dec 4, 2017

tbepdb Dec 4, 2017

gibson042 Dec 4, 2017

gibson042 Dec 4, 2017

gibson042 Dec 4, 2017

gibson042 Dec 4, 2017

gibson042 Dec 4, 2017

gibson042 Dec 4, 2017

mgol Dec 4, 2017

timmywil commented Dec 11, 2017

gibson042 Dec 11, 2017

tbepdb commented Dec 11, 2017

timmywil commented Dec 11, 2017

Manipulation: Add support script type module #3869

Manipulation: Add support script type module #3869

Conversation

tbepdb commented Dec 3, 2017 • edited by timmywil

Summary

Checklist

gibson042 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmywil commented Dec 11, 2017

Choose a reason for hiding this comment

tbepdb commented Dec 11, 2017

timmywil commented Dec 11, 2017

tbepdb commented Dec 3, 2017 •

edited by timmywil