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

Allow DDEV nodejs_version to use external file as reference for node version #6071

Open
1 task done
n8-dev opened this issue Apr 9, 2024 · 20 comments
Open
1 task done

Comments

@n8-dev
Copy link

n8-dev commented Apr 9, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem?

Currently I have a .nvmrc file, other tools in the pipeline runs the following in it's set up

nvm install $(cat .nvmrc)
nvm alias default $(cat .nvmrc)

I came across this as I noticed that the nodejs_version default, did not match my package.json engine definition.
That is already managed for other people not on DDEV and CI tooling via .nvmrc

Ultimately I'd like to not have to manage multiple places...

Describe your solution

It would be nice if I could remove the nodejs_version and DDEV would check for a .nvmrc file and take the number from there.

Allowing nodejs_version: path/to/file OR version

It could to take the version number from a defined file
It can break if you pass something wrong, that's on you.

Arguably then this is agnostic.
eg

  • .n-node-version n's file
  • .node-version many many many more version managers
  • .nvmrc nvm

Describe alternatives

  • (was initially main solution)
    It would be nice if I could remove the nodejs_version and DDEV would check for a .nvmrc file and take the number from there.

  • additional config for nvmrc_path or something perhaps to use instead or something...
    Though usually that is only in root.

Additional context

No response

@stasadev
Copy link
Member

stasadev commented Apr 9, 2024

Hi @n8-dev,

In fact, we recommend using nodejs_version, because with it you can choose any version you want:

# use the latest 20 version
nodejs_version: v20
# use the exact version
nodejs_version: 20.1.0
# use the latest whatever it is
nodejs_version: latest

nvm needs extra steps like nvm install in a post-start hook or elsewhere - and with nodejs_version it is not needed at all.

It would be nice if I could remove the nodejs_version and DDEV would check for a .nvmrc file and take the number from there.

If I understand it correctly, you need to make adjustments to your .bashrc to use .nvmrc

https://github.com/nvm-sh/nvm/blob/master/README.md#calling-nvm-use-automatically-in-a-directory-with-a-nvmrc-file

This can be done by adding a global .bashrc file to your global ~/.ddev/homeadditions directory on the host side.

@rfay
Copy link
Member

rfay commented Apr 9, 2024

If you remove nodejs_version you get the default, which is 18 in current v1.22.7 and 20 in upcoming v1.23.0

But using the version you want there is so much easier than nvm. nvm is a shell-based utility which is never very predictable.

@GuySartorelli
Copy link
Collaborator

GuySartorelli commented Apr 10, 2024

nvm needs extra steps like nvm install in a post-start hook or elsewhere - and with nodejs_version it is not needed at all.

But using the version you want there is so much easier than nvm. nvm is a shell-based utility which is never very predictable.

I think the point is not about nvm per se - it's just about how DDEV knows what version of node it should be using. The nodejs_version value is one way to do that - but if for example your project needs to be sharable with people who don't use DDEV (or in the case Nate has mentioned, you have other pipeline tools that rely on .nvmrc to determine the node version they use), you may need a .nvmrc file as well. This means you now need to maintain the correct node version in two separate locations.

What Nate is suggesting is that if nodejs_version is omitted for a project, DDEV checks for a .nvmrc file. If that file exists, it uses it to determine the correct node version. If that file doesn't exist, it still uses the global default version (currently 18, but soon to be 20)

@stasadev
Copy link
Member

Thanks @GuySartorelli, I get the idea.

There is n install auto, that looks for .n-node-version, .node-version, .nvmrc, engine label in package.json.
https://www.npmjs.com/package/n#specifying-nodejs-versions

Unfortunately we cannot use n install auto because /var/www/html is not mounted in the Dockerfile.

We could add our own logic to read the .nvmrc and other files, but it seems easy to maintain it in only two places, and it wouldn't be obvious which version of Node.js is being used by looking at the DDEV config.

@rfay
Copy link
Member

rfay commented Apr 10, 2024

Although this is a great path for folks that use nvm, nvm is just a really poor cousin to other node version managers. It's bash based, and requires loading a bash recipe into every bash shell. It's just not a great thing, and we have a nice setup as it is, with nodejs_version.

I'm going to close this for now, but happy to continue the conversation.

@rfay rfay closed this as completed Apr 10, 2024
@GuySartorelli
Copy link
Collaborator

GuySartorelli commented Apr 10, 2024

I can kinda get where stasadev is coming from - adding this functionality would be more code to support in order to remove a small amount of complexity for users. So there's a tradeoff there and if y'all don't want to make that tradeoff, that's your call.

Randy I don't get where you're coming from at all. Nvm is very popular and very common. I can understand recommending against it's use for people who are using DDEV - but the scenarios here are a) collaborating on a project with people who do not use DDEV, and b) using .nvmrc to tell automated tools what version of node to use.
These are both fairly common scenarios, and I think it goes way outside the scope of DDEV to say "don't use nvm" in those scenario. The standpoint that this isnt worth doing because you dont think nvm is a good tool doesnt mske sense to me.

@rfay
Copy link
Member

rfay commented Apr 10, 2024

Good thoughts, reopening.

@GuySartorelli
Copy link
Collaborator

Since there have been no further objections or concerns about this, I've gone ahead and created #6108 to address it.

@rfay
Copy link
Member

rfay commented Apr 16, 2024

I apologize, but I will have objections. This would be a massive and incompatible change. The current status of leaving out nodejs_version is to get the default. And the majority of people do not use nvm.

@GuySartorelli
Copy link
Collaborator

GuySartorelli commented Apr 16, 2024

I apologize, but I will have objections.

That's fair and is your prerogative as maintainer - though I wish you'd said that sooner 😅

The current status of leaving out nodejs_version is to get the default

The proposal is not to change this behaviour - it's just a shift in what "the default" refers to. But there are other ways to go about this if you want "the default" to always mean the hardcoded default version DDEV has defined in nodeps.NodeJSDefault... which I could find very little documentation for, incidentally.

If you want that default value to always be hardcoded, other options include:

  1. introduce a new config option which would be incompatible with nodejs_version. Maybe something like nodejs_use_nvmrc, which is a boolean.
  2. Since nodejs_version is a string we could give it extra values. For example:
    1. A file path - so if the value isn't a semver string, check if the string represents the relative path to a file. If so, check the contents of that file for a node verison.
    2. Some pre-defined value, such as "use_nvmrc". So if a project has nodejs_version: "use_nvmrc" we will use the value in the .nvmrc file.

I'm open to other options as well which would help achieve this goal while not introducing changes you see as being massive.

@GuySartorelli
Copy link
Collaborator

GuySartorelli commented Apr 16, 2024

And the majority of people do not use nvm.

Just out of curiousity, and it needn't affect anything about the outcome here, but I'm curious if that's something that's being tracked in the stats DDEV gets about projects, and if so, how that is being determined?

Also, if that assertion is true... this change wouldn't affect the majority of people, which should hopefully mitigate your concern of the level of impact this would have.

@rfay
Copy link
Member

rfay commented Apr 16, 2024

First, I sure do appreciate your initiative, and your skill! I'm sorry the conversation didn't get finished before you invested the work.

though I wish you'd said that sooner

Well, I did in fact close the issue, but reopened it based on your objection, but yours was the only one.

The proposal is not to change this behaviour - it's just a shift in what "the default" refers to

That's a big shift :) And more so for people who aren't using nvm in their normal workflow. I think it might not be received well. I understand the advantage for people who do use nvm.

curious if that's something that's being tracked in the stats DDEV gets about projects

DDEV tracks commands, so tracks ddev nvm. In the last week it was used 459 times. It's #46 in the list of commands, far below share, tableplus, sequelace (2194), snapshot (2029), etc. yarn is #29 with 6146. Here's the list of top commands for the last week, https://docs.google.com/spreadsheets/d/11vAFW_IDTtBqD1eyPRd7PznvfMQ6VfWWqPL2B8Y3sLg/edit?usp=sharing

@GuySartorelli
Copy link
Collaborator

GuySartorelli commented Apr 16, 2024

I think the number of people using ddev nvm is not a suitable way to track "people using nvm", or especially "people who use a .nvmrc file to declare the version of node expected to be used for their project" - especially since the use of nvm with DDEV is actively discouraged in the docs.

Regardless, let's take as a given that the number of people with a .nvmrc file which does not match the expected value of nodejs_version in their ddev config is high. I think that's not the case in reality, but lets assume it.

Are any of the suggested alternative ways to achieve this goal outlined in my comment above suitable? Note that none of my suggested alternatives will have any impact on the default versions or on existing DDEV config - they're all explicitly opt-in.

@rfay
Copy link
Member

rfay commented Apr 17, 2024

(On vacation the next few days, I'm sure we'll take this up some more. Thanks for advocating.)

@n8-dev
Copy link
Author

n8-dev commented Apr 18, 2024

I would hazard that ddev nvm usage is low in all honestly because it is (nvm) largely done "once", therefore it's destined to not be used a lot, that doesn't mean it isn't important to workflows.

It is used in a set up a "project vibe" right? and ultimately in the scope of DDEV isn't needed as a command at all... if you run your front end things in the container...

Also, the whole situation would be

  1. have an .nvmrc
  2. said .nvmrc is out of date with their project
  3. wouldn't have defined nodejs_version:
  4. would have updated their package.json engine to match the DDEV default if it exists.

And therefore be surprised by that change to pick up the file would be quite small surely... that's a lot of conditions to be met with that problem.

This would be a massive and incompatible change. The current status of leaving out nodejs_version is to get the default.

I guess this begs the question. when DDEV updates the default nodejs_version will that be a major version bump? its a breaking change as you're saying its expect to be exact.

And unless I'm upgrading my project at the exact same time I update DDEV its a breaking change now.

@n8-dev
Copy link
Author

n8-dev commented Apr 18, 2024

I think the best solution would be the following.

Allowing nodejs_version: path/to/file to take the version number from there.
It can break if you pass something wrong, that's on you.

Arguably then this is agnostic.
eg

  • .n-node-version n's file
  • .node-version many many many more
  • .nvmrc nvm

I will edit the initial issue to have this as preference.

As the more I think about it DDEV shouldn't have an opinion on what other tools I may use, its focus is on building containers for me.

If I have a config in a file, cool, it shouldn't matter further than that.

@n8-dev n8-dev changed the title Have DDEV fall back to .nvmrc if nodejs_version isn't set. Allow DDEV nodejs_version to use external file as reference for node version Apr 19, 2024
@agarzola
Copy link

agarzola commented May 9, 2024

Over the years, our team has found itself with mismatched Node.js versions declared across multiple config files for different environments, e.g. production on 14, Tugboat/staging environments on 18, GitHub Actions (for CI, tests, sometimes building deployment artifacts) using 16 and local environments (via Lando at the time) gods-know-what. This has happened time and time again across various client projects because it is an easy thing to miss. I have been on a crusade to eradicate this problem by declaring the Node.js version to be used in any given project in a .node-version file and making every environment install whatever version of Node.js is declared in that file.

That is why I support adding the ability to point DDEV to a file where the version to be used can be declared. This can take the shape of a relative path passed to the existing nodejs_version key or a separate, dedicated key (e.g. nodejs_version_file, not unlike actions/setup-node’s node-version-file key). For the record, I strongly agree with @rfay’s opinion with regard to nvm precisely because it is to session-specific and setting the default without explicitly giving it a version is not supported (as far as I can tell). Personally, I use n locally and every project I set up for our team always has a .node-version file, but I also include an .nvmrc symlink to it to satisfy those on our team who may prefer nvm.

In case anyone is wanting to solve this problem without using nvm, here is the solution I’ve arrived at which seems to work:

# .ddev/config.yaml
hooks:
  post-start:
    - exec: export N_PREFIX=$HOME && npm install --global n && n auto && hash -r
      service: web

After a ddev restart and SSHing into the DDEV environment:

agarzola@jfklibrary-web:/var/www/html$ which node
/home/agarzola/bin/node
agarzola@jfklibrary-web:/var/www/html$ node -v
v20.13.0

Ideally, however, no post-start hook would be necessary; I could just point DDEV to our .node-version file and let it handle the installation.

@rfay
Copy link
Member

rfay commented May 10, 2024

The fundamental problem with .nvmrc is that it pushes nvm onto people, and that certainly is not a majority workflow. We don't need to integrate one population's workflow onto another population.

I'm open to generalized solutions that do not push particular workflows.

@GuySartorelli
Copy link
Collaborator

Having a .nvmrc doesn't push nvm into anyone - and in fact I suspect nobody would be adding that file just for the sake of having it, I'd suspect the proposed functionality would only be used by people who already have a file that contains the version they expect nom to be.

Regardless, the current proposal isn't to hardcode anything for .nvmrc at all, but to allow developers to tell DDEV where a file is (which could have any name), so DDEV can use thst file to detect the correct npm version.

What's more it's proposed that this be opt-in. Nobody has to use it, but people can use it if it'll be useful for them.

@n8-dev
Copy link
Author

n8-dev commented May 12, 2024

I'm open to generalized solutions that do not push particular workflows.

That's what is being proposed.

Allow nodejs_version to point to a file to obtain the version from.

The file does not matter (as it which workflow it belongs to)

No workflow is being forced on anyone.

DDEV maintains no opinion on a particular workflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants