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.

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

fix(ext/node): add missing release property to node's process #18923

Merged
merged 7 commits into from
May 2, 2023

Conversation

yardenshoham
Copy link
Contributor

@yardenshoham yardenshoham commented Apr 29, 2023

Docs: https://nodejs.org/api/process.html#processrelease

import process from 'node:process';
console.log(process.release);
// outputs:
// {
//   name: "node",
//   sourceUrl: "https://nodejs.org/download/release/v18.12.1/node-v18.12.1.tar.gz",
//   headersUrl: "https://nodejs.org/download/release/v18.12.1/node-v18.12.1-headers.tar.gz"
// }

Docs: https://nodejs.org/api/process.html#processrelease

```ts
import process from 'node:process';
console.log(process.release);
// outputs:
// {
//   name: "deno",
//   sourceUrl: "https://github.com/denoland/deno/archive/refs/tags/v1.33.1.tar.gz",
//   headersUrl: "https://github.com/denoland/deno/archive/refs/tags/v1.33.1.tar.gz"
// }
```

Signed-off-by: Yarden Shoham <git@yardenshoham.com>
@yardenshoham
Copy link
Contributor Author

I made it use deno for the name and not node. Also, I used Deno's source URL instead of node's, is that fine?

@yardenshoham
Copy link
Contributor Author

I wonder if we should set name to node to be fully compatible with node. But that feels like lying

@rajsite
Copy link

rajsite commented Apr 30, 2023

I wonder if we should set name to node to be fully compatible with node. But that feels like lying

@yardenshoham if it was changed to 'deno' that wouldn't address the original issue I filed which was an exisiting npm: scoped library using the process.release === 'node' to detect environment (browser vs worker vs node). The node check would still fail (which makes that commonjs library not usable in Deno in this case).

Guess it depends on deno's intent with the node: namespace. If the intention is strictly to be a compatibility layer then I'd expect process.release to report node. Seems like Deno's intent is exisiting npm libraries can be used in Deno without updates. Not to make the node: namespace a first class API target, i.e. the browser API's are Deno's first class APIs that users should be encouraged to target, so would only focus node: on compatibility.

If I was a library author trying to support Deno and Node and needed conditional logic / environment specific behaviors then I'd be checking for the Deno global existence. But everything under the node: namespace would be trying to emulate the node environment as closely as possible. i.e. don't want to encourage deno authors to be writing against the node: namespace so Deno wouldn't expose deno-specific behavior there.

@rajsite
Copy link

rajsite commented Apr 30, 2023

I wonder if we should set name to node to be fully compatible with node. But that feels like lying

Or to put it another way. I think for node: and npm: scoped packages Deno actively wants to lie and make it look like a normal node environment to the modules.

@yardenshoham
Copy link
Contributor Author

I agree, I changed it

@kt3k
Copy link
Member

kt3k commented May 1, 2023

name: node makes sense to me as its for compatibility.

@yardenshoham Can you run ./tools/format.js to fix format issue in CI?

@yardenshoham
Copy link
Contributor Author

yardenshoham commented May 1, 2023

It failed on a flaky test

Copy link
Contributor

@lino-levan lino-levan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@kt3k kt3k merged commit cf89374 into denoland:main May 2, 2023
@yardenshoham yardenshoham deleted the process-release branch May 2, 2023 07:24
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.

npm incompatibility due to process.release.name
5 participants