Try to fix #2296 and support to get wasm file through http protocol #2297

juzi5201314 · 2020-08-26T14:17:28Z

Fix #2296

alexcrichton · 2020-08-26T18:59:25Z

Thanks! Looks like the deno CI is failing though?

juzi5201314 · 2020-08-27T02:49:09Z

Thanks! Looks like the deno CI is failing though?

Yes, I made a stupid mistake. Now it has been fixed and passed ci.

… linux, so substring and lastIndexOf will be used to get the parent directory under linux

juzi5201314 · 2020-08-27T09:25:19Z

In deno, the usual method is to get the library directly from remote.
e.g. import {...} from'https://deno.land/x/lib/mod.ts',
But Deno.readFileSync can only read wasm files from the local file system.
So I use fetch to complete the support for getting wasm files from http.

alexcrichton · 2020-08-27T19:31:45Z

cc @jakobhellermann mind taking a look at this?

RReverser · 2020-10-09T14:02:05Z

crates/cli-support/src/js/mod.rs

+                console.error(`Unsupported protocol: ${{url.protocol}}`)
+                Deno.exit(0)
+            }}
+            const wasmModule = new WebAssembly.Module(wasmCode);


You should almost never use synchronous constructors, as they can be very slow for large Wasm. Instead, use await WebAssembly.instantiate(wasmCode, imports).

^ this issue still stands. (I'm not a maintainer, but I don't think we should ever block)

ok! f71515d

zebp · 2021-06-07T15:34:47Z

Can this get some movement? This issue makes it almost impossible to publish a Deno library that uses wasm-bindgen without modifying the generated output.

RReverser · 2021-06-07T16:30:15Z

I'll admit I don't fully understand the original issue. Given that Deno supports all the required Web APIs, doesn't --target web just work in Deno?

zebp · 2021-06-07T17:38:49Z

I'll admit I don't fully understand the original issue. Given that Deno supports all the required Web APIs, doesn't --target web just work in Deno?

It almost works with the web target. Deno's fetch doesn't support fetching local files, so it can't fetch the wasm binary.

Also calling the load function directly is inconvenient.

RReverser · 2021-06-07T19:05:28Z

Deno's fetch doesn't support fetching local files, so it can't fetch the wasm binary.

Oh I see, that's unfortunate.

juzi5201314 · 2021-06-08T02:15:04Z

Glad someone saw this problem. Is there any resistance to this problem at the moment?
How can I help?

zebp · 2021-06-14T14:01:00Z

It'd be great to get some movement on this PR, this issue is a blocking one for me.

RReverser · 2021-06-14T20:40:42Z

crates/cli-support/src/js/mod.rs

+            "const url = new URL(import.meta.url)
+
+            let wasmCode = ''
+            if (url.protocol.includes('file')) {{


Nit: better to check protocols explicitly instead of "includes". Maybe change this to a switch-case?

…ration to asynchronous.

RReverser · 2021-06-15T12:11:43Z

crates/cli-support/src/js/mod.rs

@@ -341,8 +341,7 @@ impl<'a> Context<'a> {
                console.error(`Unsupported protocol: ${{url.protocol}}`)
                Deno.exit(0)
            }}
-            const wasmModule = new WebAssembly.Module(wasmCode);
-            const wasmInstance = new WebAssembly.Instance(wasmModule, imports);
+            const wasmInstance = await WebAssembly.instantiate(await WebAssembly.compile(wasmCode), imports);


You don't need compile separately.

Suggested change

const wasmInstance = await WebAssembly.instantiate(await WebAssembly.compile(wasmCode), imports);

const wasmInstance = (await WebAssembly.instantiate(wasmCode, imports)).instance;

RReverser · 2021-06-15T12:13:16Z

crates/cli-support/src/js/mod.rs

+                    }}
+                    wasmCode = await Deno.readFile(file);
+                    break
+                case 'http:':


I'm assuming you want it to work for https too?

Suggested change

case 'http:':

case 'http:':

case 'https:':

RReverser · 2021-06-15T12:14:16Z

crates/cli-support/src/js/mod.rs

+                    console.error(`Unsupported protocol: ${{url.protocol}}`);
+                    Deno.exit(0);


Better to just throw an error and give a caller a chance to handle it, rather than forcefully close the whole program.

crates/cli-support/src/js/mod.rs

RReverser · 2021-06-15T16:20:47Z

crates/cli-support/src/js/mod.rs

+                    break
+                case 'https:':
+                case 'http:':
+                    const wasm_url = import.meta.url.substring(0, import.meta.url.lastIndexOf('/') + 1) + '{module_name}_bg.wasm';


It's more reliable to use real URL resolution instead of string manipulation. You can probably reuse this code:

wasm-bindgen/crates/cli-support/src/js/mod.rs

Line 648 in 8594cae

input = new URL('{stem}_bg.wasm', import.meta.url);

Sorry and thank you.
I'm a newbie, I didn't know that URL has this kind of usage before this. So some codes used bad methods to try to get the _bg.wasm file in the same directory.
Now the code has become clean🎉

RReverser · 2021-06-15T16:21:20Z

crates/cli-support/src/js/mod.rs

+                        if (file.startsWith('/')) file = file.substr(1);
+                        file = await Deno.realPath(file + '/../{module_name}_bg.wasm');
+                    }} else {{
+                        file = file.substring(0, file.lastIndexOf('/') + 1) + '{module_name}_bg.wasm';


Same here about URL manipulation.

RReverser · 2021-06-15T16:22:10Z

crates/cli-support/src/js/mod.rs

+                    let file = new URL(import.meta.url).pathname;
+                    if (Deno.build.os === 'windows') {{
+                        if (file.startsWith('/')) file = file.substr(1);
+                        file = await Deno.realPath(file + '/../{module_name}_bg.wasm');


Why do we use ../ on Windows but not elsewhere?

Co-authored-by: Ingvar Stepanyan <[email protected]>

alexcrichton · 2021-06-18T15:58:47Z

Thanks for this1

RReverser · 2021-06-18T17:23:46Z

@juzi5201314 Didn't see the updates following the last round of reviews, thanks, code looks a lot better!

Update mod.rs

d992cf7

Update mod.rs

7d7f1a8

juzi5201314 added 6 commits August 27, 2020 15:49

Support http protocol to obtain wasm file remotely

1f1b75a

Another stupid mistake...

fb73ae2

Fix errors caused by {}

5106e9e

bg is missing

ffeda68

RealPathSync cannot be used to get the parent directory of file under…

ae249eb

… linux, so substring and lastIndexOf will be used to get the parent directory under linux

Missing readFileSync

cd74026

juzi5201314 changed the title ~~Try to fix #2296~~ Try to fix #2296 and support to get wasm file through http protocol Aug 27, 2020

RReverser reviewed Oct 9, 2020

View reviewed changes

RReverser reviewed Jun 14, 2021

View reviewed changes

juzi5201314 and others added 3 commits June 15, 2021 11:55

Merge branch 'rustwasm:master' into master

b276ff8

Use asynchronous functions instead of synchronous functions.

f71515d

Change includes to switch-case. And change the synchronous file ope…

4920e21

…ration to asynchronous.

RReverser reviewed Jun 15, 2021

View reviewed changes

Change to @RReverser suggestion.

1ffe98c

RReverser reviewed Jun 15, 2021

View reviewed changes

crates/cli-support/src/js/mod.rs Outdated Show resolved Hide resolved

RReverser reviewed Jun 15, 2021

View reviewed changes

crates/cli-support/src/js/mod.rs Outdated Show resolved Hide resolved

RReverser reviewed Jun 15, 2021

View reviewed changes

juzi5201314 and others added 4 commits June 16, 2021 11:47

throw a proper Error instance

7f4e54d

Co-authored-by: Ingvar Stepanyan <[email protected]>

Reuse url instance

1214c9d

Co-authored-by: Ingvar Stepanyan <[email protected]>

Use URL instead of previous backward writing.

76c5e82

reuse unnecessary code.

7bc87bb

alexcrichton merged commit 80da105 into rustwasm:master Jun 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to fix #2296 and support to get wasm file through http protocol #2297

Try to fix #2296 and support to get wasm file through http protocol #2297

juzi5201314 commented Aug 26, 2020

alexcrichton commented Aug 26, 2020

juzi5201314 commented Aug 27, 2020

juzi5201314 commented Aug 27, 2020

alexcrichton commented Aug 27, 2020

RReverser Oct 9, 2020

RReverser Jun 14, 2021

juzi5201314 Jun 15, 2021

zebp commented Jun 7, 2021

RReverser commented Jun 7, 2021

zebp commented Jun 7, 2021 •

edited

Loading

RReverser commented Jun 7, 2021

juzi5201314 commented Jun 8, 2021

zebp commented Jun 14, 2021

RReverser Jun 14, 2021 •

edited

Loading

juzi5201314 Jun 15, 2021

RReverser Jun 15, 2021

RReverser Jun 15, 2021

RReverser Jun 15, 2021 •

edited

Loading

RReverser Jun 15, 2021

juzi5201314 Jun 16, 2021

RReverser Jun 15, 2021

RReverser Jun 15, 2021

alexcrichton commented Jun 18, 2021

RReverser commented Jun 18, 2021

	const wasmInstance = await WebAssembly.instantiate(await WebAssembly.compile(wasmCode), imports);
	const wasmInstance = (await WebAssembly.instantiate(wasmCode, imports)).instance;

		console.error(`Unsupported protocol: ${{url.protocol}}`);
		Deno.exit(0);

Try to fix #2296 and support to get wasm file through http protocol #2297

Try to fix #2296 and support to get wasm file through http protocol #2297

Conversation

juzi5201314 commented Aug 26, 2020

alexcrichton commented Aug 26, 2020

juzi5201314 commented Aug 27, 2020

juzi5201314 commented Aug 27, 2020

alexcrichton commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zebp commented Jun 7, 2021

RReverser commented Jun 7, 2021

zebp commented Jun 7, 2021 • edited Loading

RReverser commented Jun 7, 2021

juzi5201314 commented Jun 8, 2021

zebp commented Jun 14, 2021

RReverser Jun 14, 2021 • edited Loading

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

RReverser Jun 15, 2021 • edited Loading

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

alexcrichton commented Jun 18, 2021

RReverser commented Jun 18, 2021

zebp commented Jun 7, 2021 •

edited

Loading

RReverser Jun 14, 2021 •

edited

Loading

RReverser Jun 15, 2021 •

edited

Loading