Node.js upgrade to v18 along with dependency upgrades and linting fixes #645

bjester · 2024-05-22T21:12:40Z

Description

Upgrades Node.js to v18, along with Kolibri-Tools to v0.16, Jest to v29 and Nuxt to v2.15. Additionally, this pins dependencies in order to maintain compatibility between Nuxt and kolibri-tools, applies fixes for new linting errors, and cleans up testing output through better error handling.

Issue addressed

Addresses #439

Changelog

Description: Upgrades Node.js to v18, along with Kolibri-Tools to v0.16, Jest to v29 and Nuxt to v2.15
Products impact: none
Addresses: Upgrade to NodeJS 18 #439
Components: none
Breaking: no
Impacts a11y: no
Guidance: Netlify configuration needs to be updated to use v18 as well
Description: Component error handling now use @error listener to avoid polluting test output, nor suppressing console.* in tests
Products impact: any
Addresses: n/a
Components: KImg, KTabs, KTabsList
Breaking: yes
Impacts a11y: no
Guidance: The KImg component may now emit an Error object in its @error listener when incorrectly configured, in addition to an UiEvent|Event when an image fails to load. Consumers should type check the value.

Steps to test

Run yarn run test
Verify tests pass
Run yarn run dev
Verify documentation

(optional) Implementation notes

At a high level, how did you implement this?

Upgraded Node.js
Upgraded dependencies to highest version we could support
Cleaned up test output throwing warnings and other errors
Pinned dependency resolutions as needed

Does this introduce any tech-debt items?

Yes #646

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical and brittle code paths are covered by unit tests
The change is described in the changelog section above

Reviewer guidance

Is the code clean and well-commented?
Are there tests for this change?
Are all UI components LTR and RTL compliant (if applicable)?
Add other things to check for here

After review

The changelog item has been pasted to the CHANGELOG.md

Comments

bjester · 2024-05-23T16:58:28Z

Before I actually update the changelog file, please let me know how my notes look in the PR.

bjester · 2024-05-23T18:48:45Z

Noting we must update the Node.js version in Netlify before merging

AlexVelezLl

Thank you @bjester! This looks great to me 👐, I left just some questions. And just a small note: Could you add your changelog items to CHANGELOG.md please?

lib/KImg/index.vue

lib/tabs/__tests__/KTabsList.spec.js

.github/workflows/tests.yml

AlexVelezLl · 2024-05-27T16:13:55Z

dev_docs/01_getting_started.md

- Node.js 10.x (see [Node Version Manager](https://github.com/nvm-sh/nvm))
- Yarn 1.x
+- Node.js 18.x (see [Node Version Manager](https://github.com/nvm-sh/nvm))
+- Yarn >=1.22.22


Now when I run yarn lint, yarn dev I get this error because I dont have pnpm installed:

> yarn lint yarn run v1.22.22 $ kolibri-tools lint --pattern '**/*.{js,vue,scss,less,css}' --ignore '**/node_modules/**,**/.nuxt/**,**/dist/**,**/lib/KIcon/precompiled-icons/**,**/lib/keen/**,**/docs/environment.js,**/docs/jsdocs.js' Kolibri CLI: Error: Command failed: "pnpm --version" stderr: Volta error: Could not find executable "pnpm" Use `volta install` to add a package to your toolchain (see `volta help install` for more info). Error details written to /home/alexvelezll/.volta/log/volta-error-2024-05-27_11_10_15.764.log original error message: Command failed: pnpm --version Volta error: Could not find executable "pnpm" Use `volta install` to add a package to your toolchain (see `volta help install` for more info). Error details written to /home/alexvelezll/.volta/log/volta-error-2024-05-27_11_10_15.764.log error Command failed with exit code 1.

Does the new version of kolibri-tools require pnpm to be installed? If so I think we need to add this to our prerequisites.

There should be no requirement on pnpm-- nothing in regards to that has changed.

Have you previously run a corepack command on your system?

I don't think this will resolve the issue, but you could try adding export SKIP_YARN_COREPACK_CHECK=1 to your terminal env.

Hmm, no, that didn't solve it. It's weird because I tried to run it on my Windows machine, where I also don't have Pnpm, and it worked there 🤔

In my Ubuntu machine I finally installed pnpm using volta install pnpm and then yarn lint worked.

bjester · 2024-05-28T17:50:49Z

Hmm linting was passing, but now it isn't 🤔 I'll take a look

MisRob · 2024-05-30T14:52:09Z

Before I actually update the changelog file, please let me know how my notes look in the PR.

Thank you, @bjester, to me it looks well and clear, and I appreciate the guidance

MisRob · 2024-06-05T09:47:39Z

Hi @bjester, thanks a lot - lots of work!

I've switched to this branch, then nvm use 18, then yarn install, and finally yarn dev. It seems the devserver initialization gets stuck and I can't access http://localhost:4000/ as usual

michaela@michaela:~/dev/kolibri-design-system$ yarn run dev
yarn run v1.22.17
$ npm-run-all --parallel dev-only _lint-watch-fix _api-watch pregenerate
$ yarn lint -w -m
$ chokidar "**/lib/**" -c "node utils/extractApi.js"
$ nuxt --port 4000
$ node utils/pregenerate.js
$ kolibri-tools lint --pattern '**/*.{js,vue,scss,less,css}' --ignore '**/node_modules/**,**/.nuxt/**,**/dist/**,**/lib/KIcon/precompiled-icons/**,**/lib/keen/**,**/docs/environment.js,**/docs/jsdocs.js' -w -m
ℹ  Netlify environment variables:                                     11:44:01
    NETLIFY:        undefined
    BRANCH:         undefined
    PULL_REQUEST:   undefined
    REPOSITORY_URL: undefined
ℹ Wrote environment to /home/michaela/dev/kolibri-design-system/docs/environment.js

 WARN  mode option is deprecated. You can safely remove it from nuxt.config

ℹ Wrote rst icon replacement strings to /home/michaela/dev/kolibri-design-system/docs/rstIconReplacements.txt

ℹ NuxtJS collects completely anonymous data about usage.          11:44:01 AM
  This will help us improve Nuxt developer experience over time.
  Read more on https://git.io/nuxt-telemetry

? Are you interested in participating? (Y/n) Kolibri Linter: Initializing watcher for the following patterns: **/*.{js,vue,scss,less,css}

 WARN  Could not extract docs from /home/michaela/dev/kolibri-design-system/lib/KThemePlugin.js

ℹ Wrote jsdocs to /home/michaela/dev/kolibri-design-system/docs/jsdocs.js
add:node_modules/.bin/acorn
add:node_modules/autoprefixer/node_modules/.bin/browserslist
add:node_modules/bonjour-service/node_modules/.bin/multicast-dns
...
add:node_modules/.bin/webpack-cli
add:node_modules/.bin/webpack-dev-server
Watching "**/lib/**" ..

 WARN  Could not extract docs from /home/michaela/dev/kolibri-design-system/lib/KThemePlugin.js

ℹ Wrote jsdocs to /home/michaela/dev/kolibri-design-system/docs/jsdocs.js

Can you see something problematic in the log above or have some recommendations on how to best proceed with debuggin?

MisRob · 2024-06-05T10:05:23Z

I wonder if this is related to the problem with the devserver.

yarn generate gives me this error

MisRob · 2024-06-05T10:06:32Z

Good news is that I could successfully run

yarn lint
yarn lint-fix
yarn lint-watch
yarn pregenerate
yarn dev-only
yarn precompile-svgs
yarn precompile-custom-svgs
yarn _lint-watch-fix
yarn test
yarn _api-watch

So it is only generate and dev causing trouble. I use Ubuntu.

MisRob · 2024-06-05T10:11:35Z

lib/KDateRange/KDateInput.vue

  import KTextBox from '../KTextbox';

+  let uuid = 0;


If we used the same approach from another component (instead of using uuid library), could there be any conflicts?

Only if the ID prefix wasn't already unique

MisRob · 2024-06-05T10:14:18Z

lib/KImg/index.vue

-          };
+          return defaultValue;
+        }
+        // Avoid accessing unset indices through `this.ratio`


MisRob

Also just read the code and haven't noticed any issues, just have one question about uuid

MisRob · 2024-06-05T16:10:45Z

@bjester Upgrading yarn to v1.22.22 as discussed helped me with yarn dev that works now. I still see problem with yarn generate. But we can have a look whether that's actually a regression or if it was present before (can try that out but later)

bjester · 2024-06-06T14:09:48Z

I have replicated @MisRob's issue. It seems to stem from incompatibility with hoisted postcss packages. I will look at fixing it

MisRob · 2024-06-19T03:42:56Z

I will unassign myself from the asigned reviewer role for now since everything is reviewed at this point. So that I don't block another round of review as I will be away for two weeks, back on July 8. If it's still open when I'm back and I can help, please ping me :)! Thanks.

MisRob · 2024-08-28T21:03:49Z

CHANGELOG.md

Leaving a note here that before merging it'd be good to revert CHANGELOG.md changes since the new GH action will now update the CHANGELOG.md after merging this PR by pasting the changelog from the PR description. Otherwise we'd have duplicate entries.

AlexVelezLl · 2024-11-08T13:31:23Z

Hi! According to our conversation with @rtibbles. This PR should be retargeted to the develop branch.

AlexVelezLl

Thanks @rtibbles and @bjester! This is looking good. I have ran all the package.json scripts and havent got any errors on any of them, and I have briefly checked the rendered docs pages, and havent noticed anything wrong with it. I just noted some js code examples that were incorrectly formatted.

And I was also wondering if we should include the code format commits to a .git-blame-ignore-revs file, similar to what we did in Kolibri to avoid losing the blame of those lines.

docs/pages/kcardgrid.vue

docs/pages/klistwithoverflow.vue

docs/pages/styling/index.vue

docs/pages/usekliveregion.vue

AlexVelezLl · 2024-12-09T18:10:15Z

lib/KDropdownMenu.vue

@@ -145,9 +146,11 @@
      window.removeEventListener('keydown', this.handleOpenMenuNavigation, true);
    },
    methods: {
-      handleOpen() {
+      async handleOpen() {


Was this a change made from the linter? :0

For some reason the linter didn't like the double nested $nextTick call unless it used await this.$nextTick() twice instead - so this seemed like the easiest way to workaround it. It feels like maybe a bug in the linter, but this also feels more readable, so I didn't see a concern.

… approach

…bri-format.

AlexVelezLl

Code changes looks good, all package.json scripts are working as expected and netlify deploy using node 18 was successful!! Thanks!

bjester force-pushed the nodejs-upgrade branch from bbb275d to cea58c6 Compare May 23, 2024 14:05

bjester changed the title ~~Nodejs upgrade~~ Node.js upgrade to v18 along with dependency upgrades and linting fixes May 23, 2024

bjester marked this pull request as ready for review May 23, 2024 16:57

bjester requested review from MisRob and rtibbles May 23, 2024 16:57

bjester requested a review from AlexVelezLl May 23, 2024 17:57

bjester force-pushed the nodejs-upgrade branch from b91addc to 1e3f048 Compare May 23, 2024 18:44

bjester changed the base branch from develop to release-v4 May 23, 2024 18:45

AlexVelezLl reviewed May 27, 2024

View reviewed changes

MisRob reviewed Jun 5, 2024

View reviewed changes

lib/KImg/index.vue

};

return defaultValue;

}

// Avoid accessing unset indices through `this.ratio`

Copy link

Member

MisRob Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

MisRob reviewed Jun 5, 2024

View reviewed changes

MisRob self-assigned this Jun 7, 2024

MisRob removed their assignment Jun 19, 2024

bjester mentioned this pull request Jun 25, 2024

Bump kolibri-tools, fix lint errors #672

Closed

AlexVelezLl mentioned this pull request Jul 3, 2024

Percy and jest-puppeteer environment setup for visual testing #670

Merged

9 tasks

rtibbles assigned radinamatic and unassigned radinamatic Jul 16, 2024

This was referenced Aug 19, 2024

Add $darken_ Utility Functions for Darkening Palette Colors #728

Merged

Upgrade color package #740

Open

MisRob reviewed Aug 28, 2024

View reviewed changes

AlexVelezLl linked an issue Sep 24, 2024 that may be closed by this pull request

Upgrade to NodeJS 18 #439

Closed

rtibbles assigned AlexVelezLl Oct 22, 2024

rtibbles changed the base branch from release-v4 to develop December 4, 2024 22:59

rtibbles force-pushed the nodejs-upgrade branch from 0beb457 to c06a8f1 Compare December 5, 2024 17:33

AlexVelezLl requested changes Dec 9, 2024

View reviewed changes

bjester and others added 13 commits December 9, 2024 12:36

Update packages and configuration

3206f93

Fix failing tests post-upgrade, cleap up, replace UUID with alternate…

dd2b62d

… approach

Update dev docs and CI

a5bd3d9

Use https

13e696b

Pin resolution of postcss b/c 7<>8 incompatibilities; linting fixes

0d7eaa3

Auto prettier/linting fixes

451f9bd

Update imports to use node prefix

130bfec

Ensure events have doc strings

6c89509

Update node version in publish workflow

bd399f0

Upgrade Vue to 2.7. Nuxt JS to latest v2. Swap kolibri-tools for koli…

574843f

…bri-format.

Add test dependencies.

36b8bba

Update all linting for latest version of kolibri-format.

cf75a89

Ignore mass update linting commit.

a5534f2

rtibbles force-pushed the nodejs-upgrade branch from c06a8f1 to a5534f2 Compare December 9, 2024 20:46

AlexVelezLl approved these changes Dec 9, 2024

View reviewed changes

AlexVelezLl merged commit ff73357 into learningequality:develop Dec 9, 2024
5 of 11 checks passed

learning-equality-bot bot pushed a commit that referenced this pull request Dec 9, 2024

Update CHANGELOG.md for PR #645

37b3f74

rtibbles mentioned this pull request Dec 11, 2024

Revert inadvertent changes to KLogo path during linting. #868

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node.js upgrade to v18 along with dependency upgrades and linting fixes #645

Node.js upgrade to v18 along with dependency upgrades and linting fixes #645

bjester commented May 22, 2024 •

edited by rtibbles

Loading

bjester commented May 23, 2024

bjester commented May 23, 2024

AlexVelezLl left a comment

AlexVelezLl May 27, 2024

bjester May 28, 2024

bjester May 28, 2024

AlexVelezLl May 28, 2024 •

edited

Loading

bjester commented May 28, 2024

MisRob commented May 30, 2024

MisRob commented Jun 5, 2024

MisRob commented Jun 5, 2024

MisRob commented Jun 5, 2024

MisRob Jun 5, 2024

bjester Jun 6, 2024

MisRob Jun 5, 2024

MisRob left a comment

MisRob commented Jun 5, 2024 •

edited

Loading

bjester commented Jun 6, 2024

MisRob commented Jun 19, 2024

MisRob Aug 28, 2024

AlexVelezLl commented Nov 8, 2024

AlexVelezLl left a comment

AlexVelezLl Dec 9, 2024

rtibbles Dec 9, 2024

AlexVelezLl Dec 9, 2024

AlexVelezLl left a comment

Node.js upgrade to v18 along with dependency upgrades and linting fixes #645

Node.js upgrade to v18 along with dependency upgrades and linting fixes #645

Conversation

bjester commented May 22, 2024 • edited by rtibbles Loading

Description

Issue addressed

Changelog

Steps to test

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

Reviewer guidance

After review

Comments

bjester commented May 23, 2024

bjester commented May 23, 2024

AlexVelezLl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexVelezLl May 28, 2024 • edited Loading

Choose a reason for hiding this comment

bjester commented May 28, 2024

MisRob commented May 30, 2024

MisRob commented Jun 5, 2024

MisRob commented Jun 5, 2024

MisRob commented Jun 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob left a comment

Choose a reason for hiding this comment

MisRob commented Jun 5, 2024 • edited Loading

bjester commented Jun 6, 2024

MisRob commented Jun 19, 2024

Choose a reason for hiding this comment

AlexVelezLl commented Nov 8, 2024

AlexVelezLl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexVelezLl left a comment

Choose a reason for hiding this comment

bjester commented May 22, 2024 •

edited by rtibbles

Loading

AlexVelezLl May 28, 2024 •

edited

Loading

MisRob commented Jun 5, 2024 •

edited

Loading