Initial testing setup #100
Conversation
The runner script is a little more complex than I had hoped, due to rsms/estrella#29
This should have been part of the original README update for contributing
|
|
This pull request is being automatically deployed with Vercel (learn more). microsite-examples – ./🔍 Inspect: https://vercel.com/nmoo/microsite-examples/pyqpa7wnw microsite – ./site🔍 Inspect: https://vercel.com/nmoo/microsite/nnj6wr4e0 |
|
Oh I meant to add... it wasn't entirely clear to me which bootstap command was correct, but nothing I tried could prevent the huge package-lock.json changes. |
|
This is probably a wrap for me today, it’s puppy time again. But I wanted to also add that it’s not always clear to me when Actions are accessible so I’m going to include a screenshot showing I did get the expected red x/green check on the CI run in my fork:
|
| @@ -50,7 +52,6 @@ | |||
| "cosmiconfig": "^7.0.0", | |||
| "esbuild": "^0.8.16", | |||
| "execa": "^4.0.3", | |||
| "globby": "^11.0.1", | |||
There was a problem hiding this comment.
Unless I'm mistaken, I think globby needs to stay here. I think it got hoisted into the root? There is some logic in the cli that uses it.
There was a problem hiding this comment.
Ah you're right! I'll add it back. And I'll look at adding a regression test so I don't make the same mistake again! :)
Incidentally, this is something that PNPM would catch. I've been meaning to investigate switching my projects over from Yarn since it's a dead end for me (they apply semi-mysterious patches to some dependencies for their Plug'N'Play alternative to node_modules, even if you opt out of PnP, which is also a security concern in my opinion).
There was a problem hiding this comment.
Welp, glad I wanted to write a real non-example test. Turns out this setup doesn’t support tsconfig paths, which is probably gonna cause some pain with this directory structure. So I’m also looking into the solve for that.
There was a problem hiding this comment.
Ah that would be a problem, yeah. Let me know if you get stuck.
|
|
||
| 3. Install dependencies for the monorepo: `npm install`. | ||
|
|
||
| 4. Install and link dependencies in sub-packages: `npx lerna bootstrap`. <!-- Note for review: I actually have no idea which command is preferred here, each one I've tried has still produced changes to most or all `package-lock.json` files in the project. --> |
There was a problem hiding this comment.
Hmm, I'm not positive either. I think npx lerna bootstrap --hoist?
There was a problem hiding this comment.
I tried that and it had an even bigger impact on the package locks. I'll go with that if it's what you prefer, and now it'll be documented so we're both doing the same flow!
There was a problem hiding this comment.
I think for the normal dev setup this is probably the preferred solution, in that it minimizes the hit to the file system. But I will note that it doesn’t play nice with npm install in the root package, because it wipes out most of the root lock file. This isn’t a problem I’ve had using Yarn with workspaces, because both tools know about each other’s hoist behavior. However another call to bootstrap does work. It’s just a somewhat slow operation.
There was a problem hiding this comment.
Hmm, weird. I would be open to using yarn if it would solve this, although I know you mentioned you prefer pnpm? Volta does manage your package managers (npm, yarn, etc), but it's probably best to stick with npm if possible.
Also I have my eye on turborepo to solve a lot of lerna headaches... Not the biggest lerna fan but it seems to be the least terrible option for now.
There was a problem hiding this comment.
I'm hesitant to recommend Yarn due to issues I mentioned here, even though I'm pretty familiar with it, and have even somewhat validated using it in a monorepo without any other tools. Most of my own projects are currently pinned to Yarn 1 to avoid those PnP issues, and I don't think there's a future where I'll want to migrate them to Yarn 2 because of those issues, so for my use Yarn is only viable as long as v1 is actively being maintained. The other alternative I mentioned in that comment, PNPM, also supports workspaces, so I would imagine it's also well positioned to play well in a monorepo.
I'm not familiar with Turborepo, so I can't speak to that. Another one I've wanted to look into is Rush, which is developed by Microsoft (they definitely do monorepos!), and supports NPM, PNPM, and Yarn.
| @@ -15,7 +15,9 @@ | |||
| "scripts": { | |||
| "prepare": "npm run build", | |||
| "prebuild": "node ./scripts/clean.js", | |||
| "build": "tsc -p tsconfig.json --skipLibCheck" | |||
| "build": "tsc -p tsconfig.json --skipLibCheck", | |||
| "test": "node ../../scripts/test.js --once", | |||
There was a problem hiding this comment.
Would it make more sense to move these scripts to the monorepo root and have the script be responsible for running all the packages tests? That's usually how I see Jest setup in a monorepo.
There was a problem hiding this comment.
Certainly happy to do that. I was considering doing that, but when I've worked with Jest on past projects it was so unbearably slow and false failure prone (the way it does "isolation" is horrifically stateful with any module that has side-effects on import) that I did everything I could to make it easy to run as few tests as possible during dev. I didn't even consider that case for this initial PR, so I have no problem with the naive solution being "just run everything". All the tools in use here are so fast anyway that it may even be good enough long term.
There was a problem hiding this comment.
The one concern I have with this as I’m trying to make the change is that it may not play nicely with any config difference between packages, as it’ll resolve to a different CWD. But I’ll see if I can make it work, I’m not giving up!
There was a problem hiding this comment.
Right that makes sense. I don't think I'm extending a base config between packages to keep them consistent. That's probably a good idea.
|
@eyelidlessness I took a pass at implementing |
|
@natemoo-re I'm glad you're looking at alternatives, because honestly I'm ready to throw my computer into the ocean and walk in after it, after struggling to get the changes in with Estrella. Unfortunately I'm seeing the same thing fail with I don't have any more steam for this today (which is a shame, I really thought I'd wrap up this review in ~1h and make some progress on #96 and my own project today, but if I've learned anything with frontend tooling, it's that everything is gonna not work until you wanna quit tech forever). |
|
Ugh, the tooling wormholes in Frontend are the worst. I'm going to take a pass at finishing this up so you can focus on some actual tests and not wrestling with the config. I'll be sure to write up some docs on how things are working. Thanks so much for all the legwork on this! |
|
@eyelidlessness Couldn't have gotten this working without you! I'm going to close this in favor of #104, which I got to respect ESM |
This includes one example test (which I'll delete when I get around to actually fixing #96). The runner is a bit more complex than I anticipated (due to rsms/estrella#29), but I'm quite impressed with its performance! I hope it's alright I branched off v1.1.0, it's totally fine if you want to rebase to main.