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.

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

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

Check globalThis.Deno when setting ENVIRONMENT_IS_NODE #19359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,3 +592,5 @@ a license to everyone to use it as detailed in LICENSE.)
* Alexandra Cherdantseva <[email protected]>
* Michael Schmuki <[email protected]>
* Skye Gibney <[email protected]>
* Roj <[email protected]>
* X. <[email protected]>
2 changes: 1 addition & 1 deletion src/shell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ var ENVIRONMENT_IS_WEB = typeof window == 'object';
var ENVIRONMENT_IS_WORKER = typeof importScripts == 'function';
// N.b. Electron.js environment is simultaneously a NODE-environment, but
// also a web environment.
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';
#if AUDIO_WORKLET
var ENVIRONMENT_IS_SHELL = !ENVIRONMENT_IS_WEB && !ENVIRONMENT_IS_NODE && !ENVIRONMENT_IS_WORKER && !ENVIRONMENT_IS_AUDIO_WORKLET;
#else
Expand Down
2 changes: 1 addition & 1 deletion src/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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?

if (ENVIRONMENT_IS_NODE) {
// Create as web-worker-like an environment as we can.

Expand Down