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

Incomplete version number in .nvmrc #891

Closed
weltenwort opened this issue May 31, 2018 · 6 comments
Closed

Incomplete version number in .nvmrc #891

weltenwort opened this issue May 31, 2018 · 6 comments

Comments

@weltenwort
Copy link
Member

Version: >=v0.0.27

Description:

The .nvmrc file just contains 8, which is not recognized as a valid Node.js version number by some tools (e.g. asdf). Since this file is also included in the distributed bundle, it can cause problems during installation in some environments where those tools try to select the correct Node.js version number for the postinstall script.

Possible Solutions:

  1. Replace the incomplete version number with a valid Node.js version number, e.g. 8.11.2.
  2. Exclude the .nvmrc file from the distributed bundle. This could prevent some problems during installation of the distribution, but not when working with the source release.
@weltenwort
Copy link
Member Author

Personally, I would prefer the first solution, because it should fix the problem for both repository clones and the compiled distribution.

@chandlerprall
Copy link
Contributor

IMO the .nvmrc file shouldn't be distributed. EUI's runtime has no dependency on node, and if it did the package.json's engines field could be used to include messaging about invalid environments. EUI's inclusion in other projects (like Kibana) does have a dependency on node through the postinstall script, but npm or an npm-equivalent (yarn) is executing the command, so we know node is around.

I would also hesitate to be very prescriptive about the exact version of node needed. We'd want to target Kibana's version and would need to update at the same time, but there may be other problems introduced.

.nvmrc in the repo makes it clear to developers what EUI's needs are when developing, but adds no value (that I see) in the distributed package.

@weltenwort
Copy link
Member Author

Kibana specifies its Node.js version in the a.b.c format, both in .nvmrc and .node-version.

@chandlerprall
Copy link
Contributor

Right. I'm curious what would happen (or go wrong) if Kibana uses 8.11.1 and EUI specifies 8.11.0, for example.

@weltenwort
Copy link
Member Author

Since EUI targets the browser, it seems that any mismatch problems would occur in the tooling used at install-time. I can think of three scenarios that could cause problems:

  1. EUI ships or depends on native code that only works against specific versions of Node.js.
  2. EUI's scripts depend on ECMAScript language features that are not supported anymore in Kibana's Node.js version. (Has any feature ever been removed from that language?)
  3. EUI's scripts depend on Node.js APIs that are not supported anymore in Kibana's Node.js version.

@chandlerprall
Copy link
Contributor

I think you're 3 points are correct. We don't currently have any of those limitations, and I'm not convinced .nvmrc is the right place to defend against them for published packages. I'm going to pull the nvmrc file out of the published package and it can be revisited again if/when another use case comes up.

Thanks for digging into the installation problem and filling this issue!

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

No branches or pull requests

2 participants