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

chore: set up initial actions & docs #1

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

JakobJingleheimer
Copy link
Member

No description provided.

@bmuenzenmeyer
Copy link

just throwing this previous conversation here nodejs/release-cloudflare-worker#132 - it's not a blocker, but i honestly dont know how to manage this sort of fragmentation across the project - perhaps it doesnt matter

package.json Outdated Show resolved Hide resolved
@JakobJingleheimer
Copy link
Member Author

I think some fragmentation is fine. I think it's more important to keep standards similar. How they are enforced is an implementation detail, and the existing modus operandi of the project is, without a compelling reason to the contrary, that's author's choice.

ESLint being "in the family" is maybe a compelling reason, but i think not a clear-cut one.

I don't have much preference though. ESLint has been fine when I've used it before. I only tossed in biome here because of the reasons Augustin mentioned.

@AugustinMauroy

This comment was marked as resolved.

@@ -0,0 +1,18 @@
# Contributing
Copy link
Member

Choose a reason for hiding this comment

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

Need to add code snippets about how to run test and how to write a test.
Take example on nodejs.org repo we had done huge work to have something really explicit

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea!

This PR isn't finished btw. Just as much as I could get done last night. But it's ready for input/discussion 🙂

biome.jsonc Outdated Show resolved Hide resolved
@AugustinMauroy
Copy link
Member

Should we have commit hooks ?

node-version: ${{ matrix.node-version }}
cache: 'npm'
- run: npm ci
- run: npm run lint
Copy link
Member

Choose a reason for hiding this comment

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

this should be done in sub steps ? I'm no sure.
@bmuenzenmeyer is more of an expert on this than I am

Comment on lines +4 to +5
"javascript.updateImportsOnFileMove.enabled": "always",
"typescript.updateImportsOnFileMove.enabled": "always"
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we need to set an additional property so that VS Code doesn't change d.ts.js when it updates imports. I don't recall what the config prop is though.

Copy link
Member

Choose a reason for hiding this comment

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

I've no idea, but I think this behaviour is gone now, but not for sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely not gone. It happened to me like yesterday.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@AugustinMauroy

This comment was marked as resolved.

Co-authored-by: Jacob Smith <[email protected]>
@JakobJingleheimer
Copy link
Member Author

Should we have commit hooks ?

I think no: those can get hella annoying. If a user wants to commit code that will fail, that doesn't hurt anything, so let's not get in the way—maybe they have a good reason (like sharing something incomplete).

@JakobJingleheimer
Copy link
Member Author

should we include or exclude this file ?

I think include

@JakobJingleheimer
Copy link
Member Author

Hmm, I'm not sure why this works fine locally but fails in CI

> [email protected] test:unit
> node --no-warnings --experimental-test-coverage --test-reporter=lcov --test-reporter-destination=./coverage.lcov --test-reporter=spec --test-reporter-destination=stdout --experimental-test-module-mocks --import './build/snapshots.ts' --test --test-coverage-include='recipes/**/*' --test-coverage-exclude='**/*.spec.m(j|t)s' --test-coverage-lines=0.8 './packages/*/*.spec.m(j|t)s'

node: bad option: --test-coverage-include=recipes/**/*
node: bad option: --test-coverage-exclude=**/*.spec.m(j|t)s
node: bad option: --test-coverage-lines=0.[8]

https://github.com/nodejs/userland-migrations/actions/runs/11986924657/job/33420496685?pr=1#step:5:9

@JakobJingleheimer

This comment was marked as resolved.

@bmuenzenmeyer

This comment was marked as resolved.

@JakobJingleheimer

This comment was marked as resolved.

@bmuenzenmeyer
Copy link

Hmm, I'm not sure why this works fine locally but fails in CI

it's only complaining about flags written after --test... but it's odd that the failing CI doesn't seem to exhibit a pattern across os or version.

@bmuenzenmeyer
Copy link

note

  • all windows builds that are failing have this error message 't)s'' is not recognized as an internal or external command,
  • all macos or ubuntu failures have the bad option messages

@AugustinMauroy
Copy link
Member

maybe because newest version use fs.glob or something like that ?

@JakobJingleheimer
Copy link
Member Author

I think it's not fs.glob itself (available since 22.0.0, and 23.x is failing).

Putting the quotes around the values ensures they are processed by node (should be consistent behaviour); without quotes, the shell handles the globs (inconsistent behaviour).

What could be different between local and CI? It's not OS, because CI's macos is also failing.

I wonder if it's something like double vs single quotes?

@richardlau
Copy link
Member

Tests on Node.js 20 are failing because nodejs/node#53553 is not in Node.js 20.

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.

4 participants