proposal-signals / signal-utils

MIT License
69 stars 6 forks source link

Hard to get started contributing #57

Closed justinfagnani closed 2 months ago

justinfagnani commented 2 months ago

I don't use pnpm, so getting started checking out and building this project was a bit harder for me than other standard npm projects.

npm ci failed, which at least let me know that I needed to read instructions and see what package manage this project used. I didn't see pnpm installation instructions, so I tried npm i -g pnpm, then pnpm install and pnpm start as instructed, but that just gave me this error message:

 ERR_PNPM_BAD_PM_VERSION  This project is configured to use v9.0.5 of pnpm. Your current pnpm is v9.0.6

If you want to bypass this version check, you can set the "package-manager-strict" configuration to "false" or set the "COREPACK_ENABLE_STRICT" environment variable to "0"

Is there a reason this project is configured to use a specific patch version of pnpm? I don't know where the "package-manager-strict" setting is, so I have to go read pnpm docs now...

Is there an important reason to use pnpm? This isn't an extremely large monorepo. Is the performance difference that great? Initial install took 6.6s on my machine, which doesn't feel much different than what npm would have taken.

NullVoxPopuli commented 2 months ago

this project was a bit harder for me

oh no! :( I don't want that. Hopefully things can improve!

npm

This is a two project monorepo, so if there is a way to configure npm to do the same, i can be for that (without getting in to a package manager fight 😅).

Here are the requirements though:

The nice to haves, and why a default to pnpm (Tldr: habit) - 0 disk space install (basically, because i already have the deps in my main cache elsewhere on disk, so it's all links) - easier time fixing external dependencies who define their package.json wrong (common, but irrelevant for this repo) (patch-package isn't sufficient here usually -- but packageExtensions is the feature that lets you fix missing dependencies, peers, etc -- very easy) - i use it for all my other projects -- pnpm seems like a better npm in everyway, imo, except that it doesn't come installed by default.
ramblings I do understand there is value in 'the defaults', But, it seems corepack needs some work, because it seems silly to error, and that automatically installing the configured package manager and specific version ... For this project, it probably doesn't matter, but specific versions do matter on bigger projects. I'd want to steer forks away from corepack! Or at least manual tooling management as corepack seems like a partial tool so far, i'm sure they're working on it tho. Automating all that is one of my favorite productivity hacks in OSS! (I use volta atm (not a requirement for contribution), but i'm open to switching to other stuff, this space is quite big!) At the very least the packagemanager field is package.json should probs be just the major. And we should figure out how to have corepack not error. > I don't know where the "package-manager-strict" setting is Smells like an .npmrc setting
justinfagnani commented 2 months ago

Yeah, I don't mean to start a package manager fight. It might just be that non-pnpm users don't know how to get started and so need just a little guidance.

At the very least the packagemanager field is package.json should probs be just the major.

This (and a link or inline instructions on installing pnpm) sounds like it might fix most of the friction?

fwiw, how I'd probably set this project up npm workspaces have worked well for me, although it does hoist be default which can lead to bad imports if you're not careful. I tend to use linting for that while I wait for install-strategy=linked to work for me. I'm pretty sure that the top-level package being the main package works, even though my monorepos don't have a main package. You just like the nested package in the `workspaces` setting. So I would do that, and instead of Vite and Babel, I would use plain TypeScript, node:test, and then [Wireit](url) to make sure that the package is built before the tests. I know Wireit is a non-standard tool, and an opinion, but it lets you continue to use standard npm commands while coordinating scripts. I won't suggest you actually do this :) I'm willing to get things setup on my end to contribute!
NullVoxPopuli commented 2 months ago

I've opened https://github.com/proposal-signals/signal-utils/pull/58 for feedback you have the time! would be super appreciated :tada:

> use plain TypeScript, node:test, typescript doesn't support declaration bundling (not that that's used here -- but I've kind of lost faith in tsc as a build tool...), and we test against the browser as well as node -- which is important for a cross-platform library :tada:
NullVoxPopuli commented 2 months ago

Gonna close for now, as the readme is updated, if there is we're discussion to have, lemme know and i can reopen