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

Improve the logic of isNode(). #421

Merged
merged 1 commit into from Dec 22, 2023

Conversation

tamuratak
Copy link
Contributor

Improve the logic of isNode(). Close #320.

The trick of '[object process]' seems to be widely used.

Now, since the process.versions is empty on jspm-core, you could close the original issue without merging this PR.

@josdejong
Copy link
Owner

Thanks, this approach makes sense. How about removing the logic that checks .node.versions? I think that is redundant now. It can become just nodeProcess + "" === "[object process]", right ? Or String(nodeProcess) === "[object process]" to make it a bit more explicit.


> Now, since the `process.versions` is empty on `jspm-core`, you could close the original issue without merging this PR.

Do you mean that #320 _is_ already fixed without this PR?

@tamuratak
Copy link
Contributor Author

How about removing the logic that checks .node.versions?

It would break workpool on the Electron renderer process, which has the built-in process although we should consider it as a browser platform.

Do you mean that #320 is already fixed without this PR?

Yes. This PR would be good for another polyfill library that will appear in the future.

@josdejong
Copy link
Owner

Ah, of course. In the isNodeJs function of pdf.js that you linked to there are also a couple of extra conditions to distinguish from Electron and NW I see. Ok let's keep the logic that we already had as is 👍 .

@josdejong josdejong merged commit 6417eaf into josdejong:master Dec 22, 2023
3 checks passed
@josdejong
Copy link
Owner

Published now in v9.0.4, thanks again. I wish you nice holidays!

@tamuratak tamuratak deleted the improve_isnodejs branch December 22, 2023 07:46
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.

Improve logic to determina whether in a browser or nodejs environment
2 participants