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

Hard coded check for process.release.name === 'node' causing issues #2356

Closed
nyteshade opened this issue Feb 21, 2019 · 2 comments
Closed

Hard coded check for process.release.name === 'node' causing issues #2356

nyteshade opened this issue Feb 21, 2019 · 2 comments

Comments

@nyteshade
Copy link

nyteshade commented Feb 21, 2019

9 days ago in the following commit you added a check to see if the process.release.name === 'node'. This is causing our deploys to fail because we use nsolid. Please help!

3f7a7f3#diff-c710f70502c84a5d7779daee202dbddb

@nyteshade
Copy link
Author

This was rolled into 2.3.3. Anybody using nsolid instead of node will not be able to update their version of apollo-server due to the hard string check.

abernix added a commit that referenced this issue Feb 22, 2019
Previously, in order to only load dependencies which only work on Node.js
and fail in non-Node.js environments, we introduced a check which did a
string-comparison of `process.release.name` to see if it is was `node`.

This was first introduced in 9c563fa as
part of #2054, but has gone on to be useful for other purposes as well.

Some Node.js forks such as NodeSource's N|Solid, which is a fork of Node.js
which follows-up each Node.js release with a custom build that includes
additional native addons but is otherwise the same, override this value with
their own name.  This means that N|Source returns `nsolid`, despite the fact
that it is almost entirely the same as Node.js.

Luckily, N|Solid leaves the base version of its Node.js in
`process.versions.node` (and additionally adds its own
`process.versions.nsolid`).  By relaxing the string comparison on
`process.release.name`, we should still be able to accurately detect the
environment we want - which is "Close enough to Node.js!".

Fixes #2356
abernix added a commit that referenced this issue Feb 22, 2019
Previously, in order to only load dependencies which only work on Node.js
and fail in non-Node.js environments, we introduced a check which did a
string-comparison of `process.release.name` to see if it is was `node`.

This was first introduced in 9c563fa as
part of #2054, but has gone on to be useful for other purposes as well.

Some Node.js forks such as NodeSource's N|Solid, which is a fork of Node.js
which follows-up each Node.js release with a custom build that includes
additional native addons but is otherwise the same, override this value with
their own name.  This means that N|Source returns `nsolid`, despite the fact
that it is almost entirely the same as Node.js.

Luckily, N|Solid leaves the base version of its Node.js in
`process.versions.node` (and additionally adds its own
`process.versions.nsolid`).  By relaxing the string comparison on
`process.release.name`, we should still be able to accurately detect the
environment we want - which is "Close enough to Node.js!".

Fixes #2356
@abernix
Copy link
Member

abernix commented Feb 22, 2019

Thanks for reporting this! It should be fixed in v2.4.6 via #2357. I think relaxing of the process.release.name check seems reasonable since we're already checking for the presence of the process.versions.node property, which based on my inspection of the nodesource/nsolid:carbon-latest Docker image, should still be guaranteed on nsolid.

We could consider another technique, but let's see how this works! I'm certainly open to any expert nsolid input you might have if you think there are better ways to make this determination.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants