Skip to content

Check globalThis.Deno when setting ENVIRONMENT_IS_NODE #19359

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rojvv
Copy link

@rojvv rojvv commented May 15, 2023

This makes the source codes generated work better outside Node.js.

Co-authored-by: X. [email protected]

This makes the source codes generated work better outside Node.js.

Co-authored-by: X. <[email protected]>
@rojvv rojvv changed the title Check globalThis.Deno when setting ENVIRONMENT_IS_NODE Check globalThis.Deno when setting ENVIRONMENT_IS_NODE May 15, 2023
@@ -14,7 +14,7 @@ var Module = {};

#if ENVIRONMENT_MAY_BE_NODE
// Node.js support
var ENVIRONMENT_IS_NODE = typeof process == 'object' && typeof process.versions == 'object' && typeof process.versions.node == 'string';
var ENVIRONMENT_IS_NODE = typeof Deno == void 0 && typeof process == 'object' && typeof process.versions == 'object' && typeof process.versions.node == 'string';
Copy link
Collaborator

Choose a reason for hiding this comment

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

So the goal here is not to set ENVIRONMENT_IS_NODE when running on deno?

How much will actually work in that case? Do we need ENVIRONMENT_MAY_BE_DENO to get things to actually work correctly, or does some stuff just work as long as we don't set ENVIRONMENT_IS_NODE.

What ENVIRONMENT_IS_XXX variable would be set in that case? I guess none of them? I'd be somewhat surprised if that works.

Copy link
Author

@rojvv rojvv May 15, 2023

Choose a reason for hiding this comment

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

It’s currently working fine on Deno most of the times. But sometimes the process object is made available in the Deno environment for compatibility with Node.js.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How you would feel about an opt-in -sENVIRONMENT=deno flag?

Even if we move forward without that, it seems like maybe we should add some basic tests if we are going to claim to run under deno.

Copy link
Author

Choose a reason for hiding this comment

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

I think it’d be cool. It would just need to remove the “is Node.js” checks and think as if it is a browser and everything will work fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So we should set ENVRIONMENT_IS_WEB under deno you think? Does that already get set today?

Copy link
Author

Choose a reason for hiding this comment

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

I just tried ENVIRONMENT=web and it worked. But I still think this change makes sense if we want to target the three at once. And I want to do that now in a lib of mine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do threads work under Deno via the Web's Worker API? (if not then I don't think we need to modify this version of the ENVIRONMENT_IS_NODE).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, are you saying the deno defines process.versions.node ? That seems rather odd.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, threads work under Deno via Web Worker API. No, Deno doesn’t define process.versions.node, rather as I said sometimes its environment is modified for compatibility with Node.js modules, for example by esm.sh.

Copy link
Collaborator

@sbc100 sbc100 May 16, 2023

Choose a reason for hiding this comment

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

I think trying to play cat a mouse with folks that are trying to polyfill node by defining process.versions.node inside of something that isn't node, might not be the right direction. If esm.sh is trying to claim to be compatible with node by declaring process.versions.node is seems reasonable to take them at their word... otherwise we are basically trying to tell the difference between real node and pollyfilled/fake node.

On the other hand I'm not an expert in this realm so maybe this kind of thing is normal? Do you know of the best/recommend way to detect "real" node?

@kripken
Copy link
Member

kripken commented May 15, 2023

Side note, I still think the best plan forward to support Deno, Bun, and other environments is #12203

I am not necessarily opposed to small incremental improvements like this PR in the meantime, though (but I share @sbc100 questions, above).

@rojvv
Copy link
Author

rojvv commented May 16, 2023

As I said, Deno is already supported with ENVIRONMENT=web. Any code written for browsers has a high chance to work on Deno. It has the most complete support for Web APIs compared to Bun (or the latest Node.js if you say so).

You won’t have to have any polyfills for it. Both now and in the future.

As for Bun support, all I can say right now is “Good luck.” It is still mostly in the works and isn’t completely supporting major Web APIs.

image

Also, if Bun got stable and implemented Web APIS, the only runtime that would require polyfilling would be Node.js. Right now, both Bun and Node.js might require polyfilling.

@rojvv
Copy link
Author

rojvv commented May 16, 2023

Well, I just noticed ENVIRONMENT=web works with Node.js as well.

@rojvv
Copy link
Author

rojvv commented May 16, 2023

I’m on v20. Not sure if it will work with older versions 🤔

@sbc100
Copy link
Collaborator

sbc100 commented May 16, 2023

I’m on v20. Not sure if it will work with older versions thinking

It probably depends on what features you use. I imagine that for many programs the all the code ends up being portable.

But, if you try to use something like threads and you want to run under node you would need explicit node support in the generated code.

@kripken
Copy link
Member

kripken commented May 17, 2023

That's good news. If more and more runtimes support Web APIs then maybe we can just recommend -sENVIRONMENT=web all the time. In that case #12203 might not even be needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants