Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ jobs:

steps:
- uses: actions/checkout@v4
- uses: ljharb/actions/node/install@main
name: 'nvm install lts/* && npm install'
- uses: actions/setup-node@v4
Copy link
Member

Choose a reason for hiding this comment

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

this is not a preferred change, it's actually less simple, and it's not maintained by me so tc39 has less control over it.

Copy link
Author

Choose a reason for hiding this comment

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

It's the simplest because everyone uses it, because it manages versions for us, and it hides the npm dep.

Copy link
Member

Choose a reason for hiding this comment

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

why would we want npm hidden?

Copy link
Author

Choose a reason for hiding this comment

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

my bad I mean cache.
But FIY "cache" in French mean hide. So just ADHD

Copy link
Member

Choose a reason for hiding this comment

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

my actions can do caching as well, but caching adds complexity that can hide bugs, so it's usually best avoided when speed isn't a priority, as it isn't here.

Copy link
Author

Choose a reason for hiding this comment

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

just realize that caching can be accomplish without lock file

Copy link
Author

Choose a reason for hiding this comment

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

Let's get back to the main topic, which is that action/setup-node is used by everyone and is very well maintained.
And I remain convinced that using a standard GHA is the right approach.

Copy link
Member

Choose a reason for hiding this comment

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

It is not always well maintained - there's been problems with it before - and it doesn't really matter who else uses it. We're going to keep using the same things we've been using across the tc39 org, they work well and we can directly fix issues in them, unlike in setup-node.

with:
node-version: lts/*
- run: npm run build
cache: 'npm'
- run: npm install
- run: node --run build
Copy link
Member

Choose a reason for hiding this comment

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

we can't use node --run, because it won't run pre and post scripts.

Copy link
Author

@AugustinMauroy AugustinMauroy Jun 2, 2025

Choose a reason for hiding this comment

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

You should never use these side effects to perform a task.
if you want a second step explicitly add it

Copy link
Member

Choose a reason for hiding this comment

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

That's an opinion, and one i profoundly disagree with.

Copy link
Author

Choose a reason for hiding this comment

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

In most cases, it seems obvious to me that having each step clearly written down instead of relying on magic is the best idea.

Copy link
Member

Choose a reason for hiding this comment

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

it is clearly written down, in package.json's scripts section. This is similar to how makefiles have operated for decades, it's not magic, it's what's expected.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's just that it's something voluntary. But I don't think we should base our decision on that. Because a task, a script, a step in the CI, a call to a script...

7 changes: 4 additions & 3 deletions .github/workflows/deploy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ jobs:

steps:
- uses: actions/checkout@v4
- uses: ljharb/actions/node/install@main
name: 'nvm install lts/* && npm install'
- uses: actions/setup-node@v4
with:
node-version: lts/*
- run: npm run build
cache: 'npm'
- run: npm install
- run: node --run build
- uses: JamesIves/github-pages-deploy-action@v4
with:
branch: gh-pages
Expand Down