feat: upgraded to node v18, added .nvmrc and updated workflows

[BilalQamar95]

Ticket

Upgrade Node Js from 16 to 18

Description

• Added support for node v18, updated .nvmrc and workflows.

Merge checklist:

• Any new requirements are in the right place (do not manually modify the requirements/*.txt files)
  • base.in if needed in production but edx-platform doesn't install it
  • test-master.in if edx-platform pins it, with a matching version
  • make upgrade && make requirements have been run to regenerate requirements
• make static has been run to update webpack bundling if any static content was updated
• ./manage.py makemigrations has been run
  • Checkout the Database Migration Confluence page for helpful tips on creating migrations.
  • Note: This must be run if you modified any models.
    • It may or may not make a migration depending on exactly what you modified, but it should still be run.
  • This should be run from either a venv with all the lms/edx-enterprise requirements installed or if you checked out edx-enterprise into the src directory used by lms, you can run this command through an lms shell.
    • It would be ./manage.py lms makemigrations in the shell.
• Version bumped
• Changelog record added
• Translations updated (see docs/internationalization.rst but also this isn't blocking for merge atm)

Post merge:

• Tag pushed and a new version released
  • Note: Assets will be added automatically. You just need to provide a tag (should match your version number) and title and description.
• After versioned build finishes in GitHub Actions, verify version has been pushed to PyPI
  • Each step in the release build has a condition flag that checks if the rest of the steps are done and if so will deploy to PyPi.
    (so basically once your build finishes, after maybe a minute you should see the new version in PyPi automatically (on refresh))
• PR created in edx-platform to upgrade dependencies (including edx-enterprise)
  • This must be done after the version is visible in PyPi as make upgrade in edx-platform will look for the latest version in PyPi.
  • Note: the edx-enterprise constraint in edx-platform must also be bumped to the latest version in PyPi.

[adamstankiewicz]

oof, blast from the past all the way to node 12 haha

[adamstankiewicz]

@BilalQamar95 @abdullahwaheed These changes look good to me. I also pushed up a CHANGELOG entry and version bump as a breaking release v4.0.0.

We unfortunately don't have capacity right now to merge / release / verify in edx-platform, but feel free to take this on if you'd like. Otherwise, we'll come back to this PR at a later date.

[BilalQamar95]

@BilalQamar95 @abdullahwaheed These changes look good to me. I also pushed up a CHANGELOG entry and version bump as a breaking release v4.0.0.

We unfortunately don't have capacity right now to merge / release / verify in edx-platform, but feel free to take this on if you'd like. Otherwise, we'll come back to this PR at a later date.

Hi @adamstankiewicz, based on CHANGELOG entry it seems you are expecting this to go as breaking change. Since there are no major version bumps in packages or any code changes made alongside the Node.js version update, I believe we can treat this update as a minor version update. Please let me know if I'm missing something, curious to know your rationale behind proposing a major release here.

[adamstankiewicz]

@BilalQamar95 I treated these changes as a new major version because, in the past, I've seen dropping Node.js support as causing breaking changes for some consumers when not released as a new major version. That said, we don't typically define an engines property in our package.json files so we haven't really explicitly defined what versions of Node our repos/libraries support...

Based on the guidance in this article, I believe what the author proposes for maintainers makes sense. We probably should be explicitly defining the supported Node.js version(s) in our repos/libraries with the engines property. I'd be curious to hear your thoughts, not just for this repository, but as a pattern more broadly.

• Define an engines field that declare the oldest active LTS version of Node.js (i.e., Node 18):

{
  "engines": {
    "node": "^18",
    "npm": "^9"
  }
}

• "When a Node.js version reaches end-of-life, even if your library is finished, bump a major version updating the engines field to the current oldest active LTS of Node.js."
• "Consider throwing a helpful exception if your library is used on an unsupported Node.js version."
• "Consider documenting your version support policy." See example from the aforementioned article.

Some related tweets:

• "Changing any supported dependency (peer or direct) major version is a breaking change and needs a new major version" (source)
• "Repeat with me: dropping support for old @nodejs release is a breaking change and it should be released in a major version." (source)

[question] Speaking of documentation, are you aware if we've documented our process for dealing with Node.js upgrades anywhere in the Open edX wiki, perhaps taking into account the above pattern with engines? If not, that might be some documentation worth having for future Node.js upgrades.

[BilalQamar95]

@BilalQamar95 I treated these changes as a new major version because, in the past, I've seen dropping Node.js support as causing breaking changes for some consumers when not released as a new major version. That said, we don't typically define an engines property in our package.json files so we haven't really explicitly defined what versions of Node our repos/libraries support...

Based on the guidance in this article, I believe what the author proposes for maintainers makes sense. We probably should be explicitly defining the supported Node.js version(s) in our repos/libraries with the engines property. I'd be curious to hear your thoughts, not just for this repository, but as a pattern more broadly.

• Define an engines field that declare the oldest active LTS version of Node.js (i.e., Node 18):

{
  "engines": {
    "node": "^18",
    "npm": "^9"
  }
}

• "When a Node.js version reaches end-of-life, even if your library is finished, bump a major version updating the engines field to the current oldest active LTS of Node.js."
• "Consider throwing a helpful exception if your library is used on an unsupported Node.js version."
• "Consider documenting your version support policy." See example from the aforementioned article.

Some related tweets:

• "Changing any supported dependency (peer or direct) major version is a breaking change and needs a new major version" (source)
• "Repeat with me: dropping support for old @nodejs release is a breaking change and it should be released in a major version." (source)

[question] Speaking of documentation, are you aware if we've documented our process for dealing with Node.js upgrades anywhere in the Open edX wiki, perhaps taking into account the above pattern with engines? If not, that might be some documentation worth having for future Node.js upgrades.

Yes that definitely makes sense, defining the supported Node.js version(s) in the engines field of the package.json file appears to be a good practice for maintaining repositories and libraries, especially in a multi-package, microfrontend environment like ours where it can serve to provide clear guidance to developers and users about required Node.js and npm versions for proper functionality and compatibility. Declaring the oldest active LTS version of Node.js in the engines field allows devs to use latest LTS version while ensuring compatibility with older versions, whereas updating the engines field to newest LTS version when an older LTS version reaches end-of-life could help ensure compatibility and security.
There is merit to consider it as a pattern broadly, I was wondering if we should create a ticket for it for now and take this up separately across all repos/libraries.

I will be updating the PR and we can release this as a major version, also we have documented the process for node v18 upgrade which you can find here.

[adamstankiewicz]

There is merit to consider it as a pattern broadly, I was wondering if we should create a ticket for it for now and take this up separately across all repos/libraries.

@BilalQamar95 I filed a new issue as an epic to define & track any work related to this updated pattern, should the team decide to adopt it more broadly: openedx/wg-frontend#171

Wasn't sure if FED-BOM prefers a single issue to track across all repos/libraries or if the team prefers to have a separate issue per repo/library; just created a single epic issue for now 😄

If the team adopts the pattern more broadly, it might be worth prioritizing this pattern given its closely related to the ongoing Node 18 upgrade work. Just note that when adding the engines property, it should probably be treated as a breaking change for libraries, resulting in a new major version.

[adamstankiewicz]

LGTM. Thanks for adding in the engines field and releasing it as a new major version.

@BilalQamar95 Will you be planning on releasing it to PyPi after merge and upgrading to it in edx-platform? Or is that something enterprise-titans should plan on tackling after this PR merges?

[BilalQamar95]

LGTM. Thanks for adding in the engines field and releasing it as a new major version.

@BilalQamar95 Will you be planning on releasing it to PyPi after merge and upgrading to it in edx-platform? Or is that something enterprise-titans should plan on tackling after this PR merges?

@adamstankiewicz we don't have any plans or the required domain knowledge to release it to PyPi, we intend to rely on enterprise-titans to tackle this once PR is merged.