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

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

Merged
merged 4 commits into from
Jun 24, 2019
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
149f303
Add WorkerShim
costingeana Jun 13, 2019
d870ed2
Add support for import maps in Worker
Jun 18, 2019
398afbb
Add newline at the end of files.
costingeana Jun 24, 2019
f73590b
1. Separate tests for WorkerShim => created test-worker.html file
costingeana Jun 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 38 additions & 7 deletions dist/es-module-shims.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,10 @@
throw new Error('Unable to resolve bare specifier "' + id + (parentUrl ? '" from ' + parentUrl : '"'));
}

function createBlob (source) {
return URL.createObjectURL(new Blob([source], { type: 'application/javascript' }));
}

function analyzeModuleSyntax (_str) {
str = _str;
let err = null;
Expand Down Expand Up @@ -689,6 +693,32 @@
throw new Error();
}

class WorkerShim {
constructor(aURL, options = {type: 'classic'}) {
if (options.type !== 'module') {
return new Worker(aURL, options);
}

let es_module_shims_src = new URL('es-module-shims.js', baseUrl).href;
const scripts = document.scripts;
for (let i = 0, len = scripts.length; i < len; i++) {
if (scripts[i].src.includes('es-module-shims.js')) {
es_module_shims_src = scripts[i].src;

break;
}
}

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

Choose a reason for hiding this comment

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

Indentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


const workerOptions = {...options};
workerOptions.type = 'classic';

return new Worker(workerScriptUrl, workerOptions);
}
}

let id = 0;
const registry = {};

Expand Down Expand Up @@ -826,7 +856,6 @@
load.b = createBlob(resolvedSource + '\n//# sourceURL=' + load.r);
load.S = undefined;
}
const createBlob = source => URL.createObjectURL(new Blob([source], { type: 'application/javascript' }));

function getOrCreateLoad (url, source) {
let load = registry[url];
Expand Down Expand Up @@ -913,19 +942,19 @@
return load;
}

let importMap, importMapPromise;
let importMapPromise;
if (typeof document !== 'undefined') {
const scripts = document.getElementsByTagName('script');
for (let i = 0; i < scripts.length; i++) {
const script = scripts[i];
if (script.type === 'importmap-shim' && !importMapPromise) {
if (script.src) {
importMapPromise = (async function () {
importMap = parseImportMap(await (await fetch(script.src)).json(), script.src.slice(0, script.src.lastIndexOf('/') + 1));
self.importMap = parseImportMap(await (await fetch(script.src)).json(), script.src.slice(0, script.src.lastIndexOf('/') + 1));
})();
}
else {
importMap = parseImportMap(JSON.parse(script.innerHTML), baseUrl);
self.importMap = parseImportMap(JSON.parse(script.innerHTML), baseUrl);
}
}
// this works here because there is a .then before resolve
Expand All @@ -938,18 +967,20 @@
}
}

importMap = importMap || { imports: {}, scopes: {} };
self.importMap = self.importMap || { imports: {}, scopes: {} };

async function resolve (id, parentUrl) {
parentUrl = parentUrl || baseUrl;

if (importMapPromise)
return importMapPromise
.then(function () {
return resolveImportMap(id, parentUrl, importMap);
return resolveImportMap(id, parentUrl, self.importMap);
});

return resolveImportMap(id, parentUrl, importMap);
return resolveImportMap(id, parentUrl, self.importMap);
}

self.WorkerShim = WorkerShim;

}());
6 changes: 5 additions & 1 deletion src/common.js
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,4 +177,8 @@ export function resolveImportMap (id, parentUrl, importMap) {

export function throwBare (id, parentUrl) {
throw new Error('Unable to resolve bare specifier "' + id + (parentUrl ? '" from ' + parentUrl : '"'));
}
}

export function createBlob (source) {
return URL.createObjectURL(new Blob([source], { type: 'application/javascript' }));
}
18 changes: 10 additions & 8 deletions src/es-module-shims.js
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { resolveIfNotPlainOrUrl, baseUrl as pageBaseUrl, parseImportMap, resolveImportMap } from './common.js';
import { resolveIfNotPlainOrUrl, baseUrl as pageBaseUrl, parseImportMap, resolveImportMap, createBlob } from './common.js';
import { analyzeModuleSyntax } from './lexer.js';
import { WorkerShim } from './worker-shims.js';

let id = 0;
const registry = {};
Expand Down Expand Up @@ -139,7 +140,6 @@ async function resolveDeps (load, seen) {
load.b = createBlob(resolvedSource + '\n//# sourceURL=' + load.r);
load.S = undefined;
}
const createBlob = source => URL.createObjectURL(new Blob([source], { type: 'application/javascript' }));

function getOrCreateLoad (url, source) {
let load = registry[url];
Expand Down Expand Up @@ -226,19 +226,19 @@ function getOrCreateLoad (url, source) {
return load;
}

let importMap, importMapPromise;
let importMapPromise;
if (typeof document !== 'undefined') {
const scripts = document.getElementsByTagName('script');
for (let i = 0; i < scripts.length; i++) {
const script = scripts[i];
if (script.type === 'importmap-shim' && !importMapPromise) {
if (script.src) {
importMapPromise = (async function () {
importMap = parseImportMap(await (await fetch(script.src)).json(), script.src.slice(0, script.src.lastIndexOf('/') + 1));
self.importMap = parseImportMap(await (await fetch(script.src)).json(), script.src.slice(0, script.src.lastIndexOf('/') + 1));
})();
}
else {
importMap = parseImportMap(JSON.parse(script.innerHTML), pageBaseUrl);
self.importMap = parseImportMap(JSON.parse(script.innerHTML), pageBaseUrl);
}
}
// this works here because there is a .then before resolve
Expand All @@ -251,16 +251,18 @@ if (typeof document !== 'undefined') {
}
}

importMap = importMap || { imports: {}, scopes: {} };
self.importMap = self.importMap || { imports: {}, scopes: {} };

async function resolve (id, parentUrl) {
parentUrl = parentUrl || pageBaseUrl;

if (importMapPromise)
return importMapPromise
.then(function () {
return resolveImportMap(id, parentUrl, importMap);
return resolveImportMap(id, parentUrl, self.importMap);
});

return resolveImportMap(id, parentUrl, importMap);
return resolveImportMap(id, parentUrl, self.importMap);
}

self.WorkerShim = WorkerShim;
27 changes: 27 additions & 0 deletions src/worker-shims.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { baseUrl as pageBaseUrl, createBlob } from './common.js';

export class WorkerShim {
constructor(aURL, options = {type: 'classic'}) {
if (options.type !== 'module') {
return new Worker(aURL, options);
}

let es_module_shims_src = new URL('es-module-shims.js', pageBaseUrl).href;
const scripts = document.scripts;
for (let i = 0, len = scripts.length; i < len; i++) {
if (scripts[i].src.includes('es-module-shims.js')) {
es_module_shims_src = scripts[i].src;

break;
}
}
Copy link
Owner

@guybedford guybedford Jun 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.


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

const workerOptions = {...options};
workerOptions.type = 'classic';

return new Worker(workerScriptUrl, workerOptions);
}
}
60 changes: 59 additions & 1 deletion test/browser-modules.js
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,4 +219,62 @@ suite('wasm', () => {
const m = await importShim('/test/fixtures/wasm/example.wasm');
assert.equal(m.exampleExport(1), 2);
});
});
});

suite('Worker', () => {
test(`should create a worker type=classic and then receive a message containing the string 'classic'`, async () => {
const worker = new WorkerShim("./fixtures/worker/clasic-worker.js", {
name: 'classic-worker'
});

const result = await new Promise((resolve, reject) => {
// set a timeout to resolve the promise if the worker doesn't 'respond'
const timeoutId = setTimeout(() => {
resolve(null);
}, 2000);

worker.onmessage = (e) => {
clearTimeout(timeoutId);

resolve(e.data);
};

worker.onerror = (e) => {
clearTimeout(timeoutId);

resolve(null);
}
});

assert.equal(result, 'classic');
});

test('should create worker type=module and then receive a message containing the result of a bare import', async () => {
const worker = new WorkerShim("./fixtures/worker/module-worker.js", {
type: 'module',
name: 'test_import_map',
importMap: self.importMap
});

const result = await new Promise((resolve, reject) => {
// set a timeout to resolve the promise if the worker doesn't 'respond'
const timeoutId = setTimeout(() => {
resolve(null);
}, 2000);

worker.onmessage = (e) => {
clearTimeout(timeoutId);

resolve(e.data);
};

worker.onerror = (e) => {
clearTimeout(timeoutId);

resolve(null);
}
});

assert.equal(result, 4);
});
});
1 change: 1 addition & 0 deletions test/fixtures/worker/clasic-worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
self.postMessage('classic');
3 changes: 3 additions & 0 deletions test/fixtures/worker/module-worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import test from "test";

self.postMessage(test);
2 changes: 1 addition & 1 deletion test/test.html
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@costingeana costingeana Jun 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. 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).
  2. 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".

Copy link
Owner

Choose a reason for hiding this comment

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

Ah I see what you mean, thanks.

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

<script type="module">
mocha.setup('tdd');
mocha.allowUncaught();
Expand Down