Upgrade Node and Npm version for edxapp

[aht007]

Desc

This PR updates the Node and npm version for edxapp pipeline to 16 and 8.5.x respectively
Changes have been tested with sandbox
Related PR for adding Node16 support in edx-platform has already been merged.

[aht007]

fsevents is an optional dependency in the edx-platform and that has been a pain point for us for a while and has hindered the node16 upgrade process. I tried fixing all the dependency conflicts in platform and regenerating the package-lock file but still we faced the fsevents error. Skipping optional dependencies looks like a potential fix for us here but npm ci doesn't allow --no-optional flag with it so we had to change that to npm install to use the --no-optional flag with it

[davidjoy]

Hmm... I'm not sure I understand. Optional dependencies aren't the same as peer dependencies, and I don't see fsevents listed as an optional dependency. I don't think we actually make use of optionalDependencies in edx-platform's package.json. I'm not sure this will do what we're hoping it will.

I thought we were looking at using legacy-peer-deps? Did we decide that was a bad idea?

[aht007]

optionalDependencies are listed in package-lock file and fsevents is mentioned as optional dependency there. Also using this command resolved the issue with sandbox build failure. Here is the link to successful sandbox creation with configuration changes from this PR. Also using legacy-peer-deps didn't seem fine to me as it will generate an invalid dependency graph and others will also have to use that flag to keep the behavior consistent as mentioned in this discussion too

[davidjoy]

Does this solution require everyone who does an npm install to know to use the --no-optional flag? I'm not sure I understand the implications for local development, as part of a prod build, in devstack or tutor, CI/CD pipelines, etc. I haven't really used the --no-optional flag before so I'm not sure exactly what it's doing. 🤔

[aht007]

No this issue of fsevents doesn't arise locally. This is only happening for most people in edx on pipelines and I faced this issue while building a sandbox with node16 config even though I had installed dependencies locally with node16 and I didn't get this issue.

[aht007]

Okay so related to your previous comment I have tried one more thing to remove --no-optional flag and try with just npm install. I was able to build a sandbox with this configuration. The reason for this according to me is that npm ci tries to install packages from package-lock file and there we have fsevents as optional dependency whereas npm install uses the package.json file to install the dependencies and as fsevents is not listed there it succeeds in installing dependencies. According to my theory it also doesn't try to install fsevents because:

• fsevents is a macOS package and also an optional dependency
• We are on debian os there and not darvin in contrast to our local setup where we are using MacOS

Previusly fsevents was being listed and installed as we are generating pacakage-lock file from MacOS and then using that package-lock file using npm ci to install dependencies

[NIXKnight]

The changes look good to me. Also @aht007 has mentioned that he has tested the run on Sandbox which further validated that the changes are not breaking anything.

[NIXKnight]

I have made some changes to this PR. I have added some environment variables for custom NODE_PATH where we can pin the version of npm without downgrading/upgrading the npm version that comes with the official nodejs apt package. I have also updated the .npmrc in this regard for a custom prefix.

[davidjoy]

These changes sound reasonable to me, though admittedly this is the first time I'm seeing many of these files here in configuration. I'm unable to comment on whether these are the 'right' set of variables for us to create and will trust your judgment. 😄

Do these changes only effect edX.org? Or would it make sense to have the Build-Test-Release working group review as well? If these files are used by the community/Tutor, I'd recommend having them take a look.

[NIXKnight]

These changes sound reasonable to me, though admittedly this is the first time I'm seeing many of these files here in configuration. I'm unable to comment on whether these are the 'right' set of variables for us to create and will trust your judgment. smile

Do these changes only effect edX.org? Or would it make sense to have the Build-Test-Release working group review as well? If these files are used by the community/Tutor, I'd recommend having them take a look.

These changes don't only effect edx.org. Anyone using the master branch will get these changes. You can have it reviewed by Build-Test-Release. However I doubt that this configuration repo is used by Tutor in any way.

[jmbowman]

If we generate package-lock.json on Linux instead of macOS, does that remove the fsevents dependency without needing to switch from npm ci to npm install? That seems like the correct environment in most cases anyway (devstack runs in Linux containers), and would preserve consistency in installed versions across environments.

[aht007]

@jmbowman We tried to do that by on a ubuntu machine but that didn't work because fsevents was still listed there somehow coming as an optional dependency of a peer dependency. One workaround was to delete the file totally and regenerate that from scratch and also upgrade the packages but that gives rise to a lot of dependency conflicts. Resolving those conflicts is a big job since all the dependencies in platform are pretty much outdated and updating them gets out of scope of this ticket as well as generates more changes to the code as well.

[davidjoy]

To be clear, the option where we delete the package-lock.json file and resolve the peer dependency issues is the "right" solution here. Everything else is just trying to find ways of avoiding the issue because of scope, and should be considered a short term workaround. Eventually we need to do this "right".

[ghassanmas]

@aht007 Try this exact steps:

1. rm pacakge-lock.json
2. Add this to package.json

       "optionalDependencies": {
       "fsevents": "1.*.*"
     }

3. rm -rf node_modules (not sure why or if this step is always needed)
4. npm install --no-optional
5. npm ci --no-optional

• I tried 1 to 4 on Node 12/npm 6 and then npm ci --no-optional with node 16, and fsevents was skipped.
• ci does take --no-optional as an option but is only effective if pacakge-lock was created with --no-optional.
  • npm install --no-optional only works if pkacage-lock.json doesn't exists, otherwise it would be ignored.

I would have openen a PR to apply those changes but I guess it would miss with your current PR openedx/openedx-platform#30274, so better if you just apply on top of it, (assuming its gonna work and folks here think its better alternative than npm install `:sweat_smile:)

[aht007]

@aht007 Try this exact steps:

1. rm pacakge-lock.json
2. Add this to package.json

       "optionalDependencies": {
       "fsevents": "1.*.*"
     }

3. rm -rf node_modules (not sure why or if this step is always needed)
4. npm install --no-optional
5. npm ci --no-optional

• I tried 1 to 4 on Node 12/npm 6 and then npm ci --no-optional with node 16, and fsevents was skipped.

• ci does take --no-optional as an option but is only effective if pacakge-lock was created with --no-optional.

  • npm install --no-optional only works if pkacage-lock.json doesn't exists, otherwise it would be ignored.

I would have openen a PR to apply those changes but I guess it would miss with your current PR openedx/edx-platform#30274, so better if you just apply on top of it, (assuming its gonna work and folks here think its better alternative than npm install `😅)

@ghassanmas
Our goal here is to move towards Node16 and Npm8. For that purpose we will have to install the dependencies with Npm 8 locally too and that will move the package-lock file to version 2. Now there are two scenarios:

1. If we remove package-lock file first and install Dependencies with Node12 it will keep the package-lock file to version 1 which is not our goal here. Secondly if we go this approach then we be having Node version at 16 and npm at version 6 for local development and if anyone updates the package-lock file with Npm 8 in edx-platform by mistake then it will cause the same issues that we are facing now with npm ci.
2. If we remove package-lock file first and try to install Dependencies with Node16 it will cause issues because we have several dependency conflicts in our package.json file and hence we cannot delete package-lock in this case. Resolving the conflicts is a big job as I said previously and will also need further changes in the edx-platform codebase.

[ghassanmas]

Firstly: you might be able in the final step to use npm 16/8, I didn't meant to say you have to use npm 6 with node /16 in the last step

Secondly: Regarding the workflow, let's imagine fsevents issue doesn't exist, how would or were you able to migrate to pacakge-lock v2/node16/npm 8 without having to deal with the peer depedency issue you mentioned?

if you are able to do that if fsevents doesn't exists, then I suppose it shall be possible to do by forcing fsevets to be optional.
The top of head approach I would try would be, before you already migrate to node/16, follow the steps above from 1-4, and then follow your steps to migrate... and then at this point I suppose you shall end up with package-lock v2 along with fsevents marked as optional?

[aht007]

@ghassanmas
Regarding your first comment I did the exact same steps and then tried building a sandbox with it using npm ci in the configuration but it still failed due to the fsevents issue.
In case you have access you can see the logs here
About the second thing that you mentioned related to upgrading to version 2 of lockfile, If we delete the lockfile and regenerate it then we face the dependency issues but if we have an existing package-lock and we just try npm install on Npm 8 then it just rewrites the file according to version2 conventions and doesn't actually change any dependencies or try to resolve conflicts and so we don't get any issue.
And one more thing I would like to mention is that fsevents is already an optional dependency in package-lock and hence listing it as optional in package.json doesn't change anything.

[ghassanmas]

@aht007... you might be right about not having to add fsevents in package.json... I am not sure... however I am certian that node/npm had some bugs regarding --no-optional as what it seems when doing research online... so can you please try to upgrade your node 12/npm 6 to minor version, e.g. node 12.22 / npm 6.14... and then try again to see if you would get it to work...

[aht007]

@ghassanmas
I think you missed the point that I raised. Adding fsevents as optional dependency is not going to resolve anything for us as fsevents is also listed as sub dependency of some other dependencies. For reference I am attaching the picture. It will still be installed due to being listed with these dependencies even if we mark it as optional in our package.json file
To resolve this issue completely we will have to update our packages that are bringing this dependency in package-lock file and then we can get rid of this issue but right now we are in no luck to update the related packages because they will bring a lot of changes as I last discussed with @davidjoy What my suggestion is that we should merge the changes with npm install as it would be harmless and then we should resolve the conflicts in edx-platform and then we can again use npm ci here. Meanwhile with this approach we will be unblocked on this PR and move to Node16
I don't see using Minor versions resolving issues for us but if you are so sure about that can you please make a PR with your changes and then ping me about that so I can create a sandbox with that and test your theory.

[ghassanmas]

@aht007

I think you missed the point that I raised. Adding fsevents as optional dependency is not going to resolve anything for us as fsevents is also listed as sub dependency of some other dependencies. For reference I am attaching the picture. It will still be installed due to being listed with these dependencies even if we mark it as optional in our package.json file

When I was first tackling this issue, as I remember running npm ls fsevents the output wasn't right or at least not what is expected, i.e. not all all instance of fsevents was marked as optional... That's why I suggested to delete package-lock.json, and to rermark it explicitly as optional.

On using npm ci vs npm installl

I don't personally, have any preference here, to weather to use npm ci npm install and I don't think it will affect tutor¹ (the main reason I guess @davidjoy raised it to community). I do beleive npm ci is more approapite use for CI/CD envs, since its faster and its what it was built for, but any ayway our disucssion here isn't really about what shall we use, it seems more about what our best option at this time

That being said, if you gonna merge this npm install I would suggest at least to make sure studio-frontend is resolved, because when using npm install npm is going to install the latest minor versions of some pacakges, where studio-frontend has some issues in its latest minor releases, hence in tutor the solution was to pin it openedx/openedx-platform/pull/30309 and also the original cause isn't merged yet openedx-unsupported/studio-frontend/pull/341 (which I guess you just commented on)

Lastly, I could try to debug, but the thing is I have other urgent stuff on my plate now, and I guess you are on a hurry to be done with this. Also If I find time for it, I would like to know if edx-platform will support node 12/14/16 or just node 16, and like how do you make sure after doing the changes it will not break, does running npm ci on node 16 is enough?.

Footnotes

1. Though there should be a consistency between what edx and tutor (tutor uses npm install) are using, but I guess that is something outside the scope of this PR, and should be discussed broadly somewhere else, and I will soon open an issue about this and tag this PR as somehow relevant. ↩

[aht007]

@aht007

I think you missed the point that I raised. Adding fsevents as optional dependency is not going to resolve anything for us as fsevents is also listed as sub dependency of some other dependencies. For reference I am attaching the picture. It will still be installed due to being listed with these dependencies even if we mark it as optional in our package.json file

When I was first tackling this issue, as I remember running npm ls fsevents the output wasn't right or at least not what is expected, i.e. not all all instance of fsevents was marked as optional... That's why I suggested to delete package-lock.json, and to rermark it explicitly as optional.

On using npm ci vs npm installl

I don't personally, have any preference here, to weather to use npm ci npm install and I don't think it will affect tutor1 (the main reason I guess @davidjoy raised it to community). I do beleive npm ci is more approapite use for CI/CD envs, since its faster and its what it was built for, but any ayway our disucssion here isn't really about what shall we use, it seems more about what our best option at this time

That being said, if you gonna merge this npm install I would suggest at least to make sure studio-frontend is resolved, because when using npm install npm is going to install the latest minor versions of some pacakges, where studio-frontend has some issues in its latest minor releases, hence in tutor the solution was to pin it openedx/edx-platform/pull/30309 and also the original cause isn't merged yet openedx/studio-frontend/pull/341 (which I guess you just commented on)

Lastly, I could try to debug, but the thing is I have other urgent stuff on my plate now, and I guess you are on a hurry to be done with this. Also If I find time for it, I would like to know if edx-platform will support node 12/14/16 or just node 16, and like how do you make sure after doing the changes it will not break, does running npm ci on node 16 is enough?.

Footnotes

1. Though there should be a consistency between what edx and tutor (tutor uses npm install) are using, but I guess that is something outside the scope of this PR, and should be discussed broadly somewhere else, and I will soon open an issue about this and tag this PR as somehow relevant. ↩

@ghassanmas
As you said when I do npm ls fsevents without any changes to the repo then I can see multiple dependencies that require fsevents but when I follow the steps you mentioned and install with --no-optional then I still have fsevents in package-lock file but I can't see any output for npm ls fsevents and still sandbox builds fail with it too. Also I tried this on a new PR with npm ci and npm ci --no-optional both so nothing worked from this end.
Also I can see that this openedx/openedx-platform#30309 PR that you mentioned is just back ported to maple branch so maybe we can just pin the studio-frontend in our Node Upgrade PR for the master branch and potentially avoid this issue and go ahead with our upgrade work
About the Node versions we will be initially supporting all the three(12, 14, 16) versions of node but after moving to Node16 on prod and verifying that everything works we will be soon dropping support for older versions and just keep Node16 in the CI

[ghassanmas]

@aht007 can you try with this openedx/openedx-platform/pull/30379 just make sure to use npm ci --no-optional

[aht007]

@ghassanmas
I used the below configurations specifying your repository for edxapp and using your branch for edxapp version and used configurations from this PR where I have updated to use npm ci --no-optional

Below is the output for this sandbox where it failed again with the same error. You can have a look at the logs and if you have acces then you can also see the sandbox here https://tools-edx-jenkins.edx.org/job/sandboxes/job/CreateSandbox/46659/

[ghassanmas]

What is the node/npm version this ran on?

[aht007]

What is the node/npm version this ran on?
Node 16 and npm 8

[ghassanmas]

Okay I think I forgot to include my latest local changes, I just pushed again, will it rerun?

[aht007]

No I had to re trigger the sandbox creation which I did last night but it still failed with the same error

[aht007]

Also there is this issue with this approach that if we even succeed in building a sandbox(which has not been the case till now) by forcing the fsevents as optional dependency and using --no-optional flag for generating the package-lock file then again we will be at square one where I suggested using this flag and David was of the view that everyone who would be developing locally would have to know and use this flag or else we will be in trouble again if the dependencies are generated without it

[ghassanmas]

Something I can't understand, why is it trying to install it, I wonder if its the npm version that pipeline is using... can we know the exact version of npm/node that is being used. (I am just curious)...
But anyway it doesn't make sense to keep trying given also forcig --no-optional doesn't seem to be acceptle path.

[aht007]

@ghassanmas
We are using the npm and node version from this same PR. You can see that here https://github.com/openedx/configuration/pull/6716/files#diff-e0912d39d78f6572319b56e288cd622ef87b08f3b6d9ae846c058f40dc365576R1102

So we have consensus here that right now we are going to move forward with the upgrade with our old edx-platform PR and with this configuration PR having npm install in it to install the dependencies. I will make sure that we keep studio-frontend issue in view and deal with it too in the same PR. I will also create a follow up ticket in frontend team's backlog to upgrade the dependencies in edx-platform and hopefully get back to using npm ci for future as Jeremy suggested.

[ghassanmas]

From my end it seems fine, studio-frontend shouldn't be an issue any more the new/latest release incudles the bundle assesst.