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

Update Node version to 20 #2569

Closed
wants to merge 1 commit into from

Conversation

gerteck
Copy link
Contributor

@gerteck gerteck commented Aug 18, 2024

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
This PR seeks to upgrade Node.js to Node 20.

  • Relevant MB documentation on Node.js migration
  • Previous upgrading Node 14 -> 16 PR

Addresses and Fixes #2541

Anything you'd like to highlight/discuss:

Before the version update, check that

  • any deprecated syntax to be refactored
  • all user-facing functionalities are working
  • no issues with the development setup
    • Tested on fresh Ubuntu VM, node v20.16.0
  • update related GitHub Actions and workflows
    • Test updated Actions
  • docs are updated
  • Deployment to Netlify/other platforms is ok
    • Netlify (PRs 1, 2 opened for netlify quickstart repos to update node version number)
    • GH-Pages
  • Deployment via CI platforms is ok
  • Update netlify.toml

Related PRs:

Related Changes

Other:
Node.js Changelogs

To-do:

  • Test deployment via other CI platforms once MarkBind ver is updated to reflect latest patches.

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)

Upgrade Node.js to v20


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

Copy link

codecov bot commented Aug 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.86%. Comparing base (70ba5e5) to head (ccf9b9c).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2569   +/-   ##
=======================================
  Coverage   51.86%   51.86%           
=======================================
  Files         127      127           
  Lines        5474     5474           
  Branches     1201     1201           
=======================================
  Hits         2839     2839           
  Misses       2340     2340           
  Partials      295      295           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Tim-Siu
Copy link
Contributor

Tim-Siu commented Aug 20, 2024

Hi @gerteck,

Thank you for your meticulous work on this PR!

I noticed the note about testing deployment via other CI platforms once the MarkBind version is updated to reflect the latest patches. Could you perhaps elaborate on why this testing is contingent on the MarkBind version update? I'm curious to understand what these patches entail and how they relate to the deployment testing.

@gerteck
Copy link
Contributor Author

gerteck commented Aug 20, 2024

Could you perhaps elaborate on why this testing is contingent on the MarkBind version update? I'm curious to understand what these patches entail and how they relate to the deployment testing.

Sure!

I was referencing the patch in PR #2562 , which in short fixes markbind deploy without the explicit need to set the cache_dir as per #2547. Previously, markbind deploy would fail on deployment outside of GitHub Actions. This patch is necessary for testing in the ci/cd workflow, since we essentially do:

npm i -g markbind-cli
markbind build
markbind deploy --ci

ciexample

That said, I suppose a hacky workaround would be to explicitly set env variables (specifically cache_dir) etc in the CI platforms to get around this issue, ceteris paribus, to test the deployment before the patch is made. Though it would be quite out of context.

This issue probably wasn't detected because #2542 had patched this issue in markbind deploy, but only specifically in GitHub Action's environment, with no testing done for Travis CI, AppVeyor or other CI platforms.


The other patches aren't required for deployment testing, even #2565 (which was a considerable issue when migrating to node 20, but doesn't affect deployment since it's a dev-dependency). Also, this issue was actually also why I focused working on #2562 earlier on prior.😄

@Tim-Siu
Copy link
Contributor

Tim-Siu commented Aug 22, 2024

Hey @gerteck,

Thanks for explaining everything and for all the testing you've done. It's clear you've put in a lot of work, especially with the workaround for the cache_dir issue. I think it's a good idea to update the MarkBind version as it would certainly save efforts, and I'll check with our seniors for their thoughts.

@yucheng11122017, could you take a look at this when you have a moment? With what @gerteck has shared, it seems like updating the MarkBind version (doing a new release) that includes the fixes from PR #2562, could help with our deployment testing on different CI platforms. What do you think about doing the update?

@tlylt
Copy link
Contributor

tlylt commented Sep 4, 2024

Thanks @gerteck for your work on this PR!

As outlined in our docs, we aim to support up to the last maintenance lts, which at the moment should be Node v18 (currently on maintenance support). Jumping directly to Node v20 is probably not what we want, unless you have some reasons for this? (Sorry the issue linked was probably not accurate)

We do need to update to v18 ASAP though. If you are keen, I would suggest we close this PR and open a new one to make the changes for v18 migration.

As for your CI testing comment, I can make a release over the weekend for the few fixes you have made (TQ again).

@gerteck
Copy link
Contributor Author

gerteck commented Sep 4, 2024

As outlined in our docs, we aim to support up to the last maintenance lts, which at the moment should be Node v18 (currently on maintenance support). Jumping directly to Node v20 is probably not what we want, unless you have some reasons for this? (Sorry the issue linked was probably not accurate)

The main reason why I directly jumped to Node v20 is because GitHub actions is transitioning directly from node 16 to 20. Although it seems that it has not been actively enforced yet.

We do need to update to v18 ASAP though. If you are keen, I would suggest we close this PR and open a new one to make the changes for v18 migration.

Sure, that would be possible!

@gerteck gerteck marked this pull request as draft September 11, 2024 17:42
@gerteck gerteck changed the title Update Node version 16 to 20 Update Node version to 20 Sep 11, 2024
@gerteck
Copy link
Contributor Author

gerteck commented Sep 11, 2024

Just for reference, here is the deployment via CI before and after making the new release:

Travis CI:

  • before: see error under deploying application
  • after: see success under deploying application

AppVeyor CI:

Circle CI:

Thanks again @tlylt for making the new release.

@gerteck
Copy link
Contributor Author

gerteck commented Sep 11, 2024

Will close this PR in favor of more updated #2572

@gerteck gerteck closed this Sep 11, 2024
@gerteck gerteck mentioned this pull request Sep 11, 2024
29 tasks
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

Successfully merging this pull request may close these issues.

Update Node version to LTS Document: Node LTS (v20) and python 3.12 does not work on MarkBind install
3 participants