Skip to content

Conversation

@stronk7
Copy link
Member

@stronk7 stronk7 commented Feb 7, 2023

We need to support Node 18 from the CI tools (see MDL-75012). So
this commit provides that, basically.

The main difference is that the npm bin command is not available
anymore, and that's what we used both for:

  1. Search if a given binary has been installed (say, grunt-cli...)
  2. Launch a number of binaries (grunt, eslint, gherkin-lint....)

So here we are geting rid of all those uses, changing them by:

  1. npm list to verify if a package is already installed.
  2. npx to launch the binaries.

Also, we have removed some very-old conditional blocks in the code, that
were providing compatibility with very, very old Moodle branches, before
we used grunt and friends.

Finally, we are now installing stylelint-checkstyle-formatter if
it's detected that core has not installed it. After all, it's a CI
dependency, not a core one, so maybe the more correct thing is to
delegate its installation to the CI / testing tools.

And, basically, that is. I've tested it with both Node 16 (current
master) and Node 18 (MDL-75012) and seems to be working ok.

Let's see if it finally pass testing. It's passing locally but have got some non-sense results in the last execution @ Travis... 🤞

@stronk7 stronk7 force-pushed the node18_compatibility_and_more branch from b7583f3 to 1979ac6 Compare February 7, 2023 13:23
We need to support Node 18 from the CI tools (see MDL-75012). So
this commit provides that, basically.

The main difference is that the `npm bin` command is not available
anymore, and that's what we used both for:

1) Search if a given binary has been installed (say, grunt-cli...)
2) Launch a number of binaries (grunt, eslint, gherkin-lint....)

So here we are geting rid of all those uses, changing them by:

1) `npm list` to verify if a package is already installed.
2) `npx` to launch the binaries.

Also, we have removed some very-old conditional blocks in the code, that
were providing compatibility with very, very old Moodle branches, before
we used grunt and friends.

Finally, we are now installing stylelint-checkstyle-formatter if
it's detected that core has not installed it. After all, it's a CI
dependency, not a core one, so maybe the more correct thing is to
delegate its installation to the CI / testing tools.

And, basically, that is. I've tested it with both Node 16 (current
master) and Node 18 (MDL-75012) and seems to be working ok.

Going to run some tests now...
@stronk7 stronk7 force-pushed the node18_compatibility_and_more branch from 1979ac6 to 8c4eedd Compare February 7, 2023 13:24
Copy link
Contributor

@andrewnicols andrewnicols left a comment

Choose a reason for hiding this comment

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

These changes look good to me.

We should decide when we want to stop supporting v14 of node, but it won't be for a while - we only switched away from it 12 months ago:

commit ef74e31086bc78ab84a6fbb0fbbb408d855e5bf4
Author: Andrew Nicols <andrew@nicols.co.uk>
Date:   Fri Feb 18 08:07:49 2022 +0800

    MDL-73915 js: Switch to nodejs lts/gallium

So I think this is good to land when tests finish.

@stronk7 stronk7 force-pushed the node18_compatibility_and_more branch from 8c4eedd to 27846eb Compare February 7, 2023 14:40
@stronk7
Copy link
Member Author

stronk7 commented Feb 7, 2023

We are green now, hence merging, thanks for the review @andrewnicols !

@stronk7 stronk7 merged commit 965739e into moodlehq:master Feb 7, 2023
@stronk7 stronk7 deleted the node18_compatibility_and_more branch February 7, 2023 15:49
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.

2 participants