Skip to content

fix: use typeof self === object && self.constructor to identifier execution environment #2855

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

HerrCai0907
Copy link
Member

Use a better way to identifier exection environemt becasue try and catch will cause misleaded error message

…xecution environment

Use a better way to identifier exection environemt becasue `try` and `catch` will cause misleaded error message
@@ -976,8 +976,8 @@ export class JSBuilder extends ExportsWalker {
}
sb.push(`} = await (async url => instantiate(
await (async () => {
try { return await globalThis.WebAssembly.compileStreaming(globalThis.fetch(url)); }
catch { return globalThis.WebAssembly.compile(await (await import("node:fs/promises")).readFile(url)); }
if (typeof self === "object" && self.constructor) { return await globalThis.WebAssembly.compileStreaming(globalThis.fetch(url)); }
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this could go wrong, say, in a Worker on a server-side runtime like Cloudflare Workers, Deno, or Bun.

Do you think there's a single solution that would work with most runtimes and bundlers?

Copy link
Member Author

Choose a reason for hiding this comment

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

I found a repo to do this thing. https://github.com/flexdinesh/browser-or-node/blob/master/src/index.ts
Maybe we can get inspired from it. But I don't have so many test environment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have checked in the runtime. Everything is fine for current version.

@HerrCai0907
Copy link
Member Author

By the way, I think github's macos runner is upgraded to aarch64 architecture. math.release test always failed as in my PC.

@HerrCai0907
Copy link
Member Author

HerrCai0907 commented Jun 20, 2024

runtime check status:
Web ✅
Node ✅
Deno ✅
Bun ✅

@@ -976,8 +976,9 @@ export class JSBuilder extends ExportsWalker {
}
sb.push(`} = await (async url => instantiate(
await (async () => {
try { return await globalThis.WebAssembly.compileStreaming(globalThis.fetch(url)); }
catch { return globalThis.WebAssembly.compile(await (await import("node:fs/promises")).readFile(url)); }
const isNodeOrBun = typeof process != "undefined" && process.versions != null && (process.versions.node != null || process.versions.bun != null);
Copy link
Member

@CountBleck CountBleck Jun 20, 2024

Choose a reason for hiding this comment

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

Suggested change
const isNodeOrBun = typeof process != "undefined" && process.versions != null && (process.versions.node != null || process.versions.bun != null);
const isNodeOrBun = typeof process != "undefined" && process.versions && (process.versions.node || process.versions.bun);

Also, at some point in the future, I think the readFile method can be removed, because Node.js in recent versions has fetch built-in.

Copy link
Member Author

Choose a reason for hiding this comment

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

agree, let us wait for the new features in nodejs.

@HerrCai0907 HerrCai0907 merged commit 78963c5 into AssemblyScript:main Jun 21, 2024
14 checks passed
@HerrCai0907 HerrCai0907 deleted the fix/node-or-web branch June 21, 2024 01:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants