Add support for import maps in Worker; resolves #9 #17

costingeana · 2019-06-13T14:10:13Z

Add support for import maps in Worker. Resolves #9.

add WorkerShim class;
'expose' importMap on global object (self) in web workers and browser.

1. add WorkerShim class 2. 'expose' importMap on global object (self) in web workers and browser.

guybedford · 2019-06-16T16:09:52Z

Amazing work and thank you for the PR! I really like how simple this turned out to be.

A nice addition might be to add support for new Worker detection in the lexer and handle the rewriting automatically without needing to use WorkerShim, but I'd be happy to include that as a possible future follow-on to this PR.

@costingeana would you be able to include a simple test before we land this?

costingeana · 2019-06-17T10:05:25Z

Thank you @guybedford, but I only followed your initial idea from #9 (comment).
I just needed import maps in Workers in a project I'm working on and I used your WorkerShim proof of concept, which worked beautifully.

What's next?
Firstly I would add some tests for the current implementation.
Secondly I will implement and test your suggestion: new Worker detection in lexer.

Thank you for the feedback on this pull request; it helps a lot.

guybedford · 2019-06-17T10:59:27Z

@costingeana happy to merge here with just a test, then let's work on the lexer addition in a new PR. The lexer doesn't have any concept of identifiers and referenced identifiers vs other types of identifiers, but we can probably just work with the fact that the new keyword followed by the Worker identifier would only ever be valid in the right place (eg function (new Worker) isn't supported etc). So again we can hack around the limitations of the lexer in a well defined way hopefully here!

1. add worker-shim.js - this module 'hosts' WorkerShim class; 2. update common.js - move here and then export it the function 'createBlob' from es-modules-shims.js; 3. update es-modules-shims.js: 3.1. import WorkerShim from worker-shim.js; 3.2. expose WorkerShim on global object (i.e. self); 4. update test/ 4.1. add new test fixtures under test/fixtures/worker/ 4.2. update browser-modules.js - add 2 simple tests for WorkerShim 4.3. update test/test.html - update the 'src' of es-module-shims.js script to '../dist/es-module-shims.js' because otherwise importScripts('../src/es-module-shims.js') used in WorkerShim will fail; ../src/es-module-shims.js is a es6 module not a 'classic' script

costingeana · 2019-06-18T15:27:24Z

My apologies for the delay!
@guybedford, please let me know if there is something else I can add (or remove).

costingeana · 2019-06-20T12:33:14Z

@guybedford, is there anything else I can do to make this pull request ready for merging?

guybedford · 2019-06-22T23:14:37Z

test/test.html

@@ -19,7 +19,7 @@
  import test from "test";
  console.log(test);
 </script>
-<script type="module" src="../src/es-module-shims.js"></script>
+<script type="module" src="../dist/es-module-shims.js"></script>


Was this necessary for the tests to pass? I must admit I quite like being able to run the tests without a rebuild.

Short answer: yes, this is necessary for tests to pass.
Long answer: The importScript() method used in WorkerShim constructor fails if the script it loads has ES6 syntax; as far as I know the importScripts() method will automatically fail inside module workers.

const workerScriptUrl = createBlob(\`importScripts('${es_module_shims_src}'); self.importMap = ${JSON.stringify(options.importMap || {})}; importShim('${new URL(aURL, pageBaseUrl).href}')`);

As I said before I made this change only to see these simple WorkerShim tests pass. However, I am of the same opinion as you when you say that you like being able to run the tests without rebuild.

To overcome this situation I see 2 quick solutions:

Revert the src path to src="../src/es-module-shims.js" and add a comment to inform everybody that Worker's tests need a different path for es-module-shims script; after all there are only 2 simple tests so it doesn't make sense to alter all the other tests which are much more important (IMHO).

Add a separate html file (e.g. test/test-worker.html) which loads only the Worker tests; in this html the src to es-module-shims script will point to ../dist/es-module-shims.js".

Ah I see what you mean, thanks.

Let's go with (2) then so that this test restriction only applies to the worker tests.

guybedford

LGTM, just let me know about that comment then I think we're good to merge.

guybedford

I've made a suggestion re the pathing - let me know if that makes sense.

guybedford · 2019-06-24T12:37:15Z

dist/es-module-shims.js

+      }
+
+      const workerScriptUrl = createBlob(`importScripts('${es_module_shims_src}'); 
+    self.importMap = ${JSON.stringify(options.importMap || {})}; importShim('${new URL(aURL, baseUrl).href}')`);


Indentation.

I think I submitted by mistake this file, namely dist/es-module-shims.js. I shouldn't have done that. I apologize.

guybedford · 2019-06-24T12:39:58Z

test/test.html

@@ -19,7 +19,7 @@
  import test from "test";
  console.log(test);
 </script>
-<script type="module" src="../src/es-module-shims.js"></script>
+<script type="module" src="../dist/es-module-shims.js"></script>


Ah I see what you mean, thanks.

Let's go with (2) then so that this test restriction only applies to the worker tests.

guybedford · 2019-06-24T12:48:23Z

src/worker-shims.js

+
+          break;
+      }
+    }


Rather than this restriction, could we instead try something like -

const esModuleShimsSrc = document.currentScript && document.currentScript.src;

Then in the WorkerShim constructor, something like -

if (!esModuleShimsSrc) throw new Error('es-module-shims.js must be loaded with a script tag for WorkerShim support.');

note the above would have to be outside of the class constructor and instead in the script initialization.

@guybedford

2. Use the approach suggested by @guybedford for detecting the `es-module-shims.js` path needed in WorkerShim constructor.

guybedford · 2019-06-24T14:15:46Z

Amazing work, thank you!

costingeana · 2019-06-24T14:36:52Z

You’re welcome! And thank you, too.

By the end of this week I will come up with another pull request to add support for new Worker detection in the lexer as you suggested earlier.

Add WorkerShim

149f303

1. add WorkerShim class 2. 'expose' importMap on global object (self) in web workers and browser.

guybedford reviewed Jun 22, 2019

View reviewed changes

guybedford approved these changes Jun 22, 2019

View reviewed changes

Add newline at the end of files.

398afbb

guybedford reviewed Jun 24, 2019

View reviewed changes

1. Separate tests for WorkerShim => created test-worker.html file

f73590b

2. Use the approach suggested by @guybedford for detecting the `es-module-shims.js` path needed in WorkerShim constructor.

guybedford merged commit 998d8dd into guybedford:master Jun 24, 2019

guybedford mentioned this pull request Oct 6, 2021

importmap in Workers #206

Closed

guybedford mentioned this pull request Mar 2, 2024

Updated Firefox stats #412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for import maps in Worker; resolves #9 #17

Add support for import maps in Worker; resolves #9 #17

costingeana commented Jun 13, 2019

guybedford commented Jun 16, 2019

costingeana commented Jun 17, 2019

guybedford commented Jun 17, 2019

costingeana commented Jun 18, 2019

costingeana commented Jun 20, 2019

guybedford Jun 22, 2019

costingeana Jun 24, 2019 •

edited

Loading

guybedford Jun 24, 2019

guybedford left a comment

guybedford left a comment

guybedford Jun 24, 2019

costingeana Jun 24, 2019

guybedford Jun 24, 2019

guybedford Jun 24, 2019 •

edited

Loading

guybedford commented Jun 24, 2019

costingeana commented Jun 24, 2019

Add support for import maps in Worker; resolves #9 #17

Add support for import maps in Worker; resolves #9 #17

Conversation

costingeana commented Jun 13, 2019

guybedford commented Jun 16, 2019

costingeana commented Jun 17, 2019

guybedford commented Jun 17, 2019

costingeana commented Jun 18, 2019

costingeana commented Jun 20, 2019

guybedford Jun 22, 2019

Choose a reason for hiding this comment

costingeana Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

guybedford Jun 24, 2019

Choose a reason for hiding this comment

guybedford left a comment

Choose a reason for hiding this comment

guybedford left a comment

Choose a reason for hiding this comment

guybedford Jun 24, 2019

Choose a reason for hiding this comment

costingeana Jun 24, 2019

Choose a reason for hiding this comment

guybedford Jun 24, 2019

Choose a reason for hiding this comment

guybedford Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

guybedford commented Jun 24, 2019

costingeana commented Jun 24, 2019

costingeana Jun 24, 2019 •

edited

Loading

guybedford Jun 24, 2019 •

edited

Loading