Skip to content

setup(ci): improve #39

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

setup(ci): improve #39

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b30591a
setup(ci): improve
AugustinMauroy Jun 2, 2025
83a66b7
include lock file
AugustinMauroy Jun 2, 2025
e16d193
Revert "include lock file"
AugustinMauroy Jun 2, 2025
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
View file
Open in desktop
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 ci
- 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 •
edited
Loading

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
View file
Open in desktop
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 ci
- run: node --run build
- uses: JamesIves/github-pages-deploy-action@v4
with:
branch: gh-pages
Expand Down
2 changes: 0 additions & 2 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ jspm_packages

# Only apps should have lockfiles
yarn.lock
package-lock.json
npm-shrinkwrap.json
pnpm-lock.yaml

# Build directory
Expand Down
1 change: 0 additions & 1 deletion .npmrc
View file
Open in desktop

This file was deleted.

Loading