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

Support PNPM Monorepos #119

Merged
merged 9 commits into from
Jul 22, 2024
Merged

Support PNPM Monorepos #119

merged 9 commits into from
Jul 22, 2024

Conversation

lishaduck
Copy link
Contributor

@lishaduck lishaduck commented Jul 17, 2024

Closes: #118.

Depends on #117.
Diff (GH doesn't like stacked PRs from forks): lishaduck/typesync@fix-monorepos...pnpm

@lishaduck
Copy link
Contributor Author

Note: I haven't added tests yet. I wanted to make sure this implementation was fine first. I'm not a big fan of adding code about a different file to a service about package.jsons, but fixing that seemed a bit like the diff might... erm... explode. So I just pretended pnpm-workspace was a package.json file.

@lishaduck
Copy link
Contributor Author

Forgot to remove a console.log. I'll get it :)

@jeffijoe
Copy link
Owner

Could you rebase this PR? I merged the other one!

src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
@coveralls
Copy link
Collaborator

coveralls commented Jul 17, 2024

Coverage Status

coverage: 100.0%. remained the same
when pulling 1860518 on lishaduck:pnpm
into 7aecb96 on jeffijoe:master.

@lishaduck
Copy link
Contributor Author

lishaduck commented Jul 17, 2024

Yay! I found a bug in npm 🤣

@lishaduck
Copy link
Contributor Author

Currently, it works...
well, CI fails.

I was going to say...
It works, but it's not polished. It currently grabs all the package.json files multiple times at least. I did drop some old polyfills at least 😁

@lishaduck
Copy link
Contributor Author

Ok, it works locally, but apparently Node 22.5 broke npm: npm/cli#7657

@lishaduck lishaduck force-pushed the pnpm branch 2 times, most recently from c14756e to da19f15 Compare July 18, 2024 03:22
src/__tests__/package-json-file-service.test.ts Outdated Show resolved Hide resolved
src/__tests__/type-syncer.test.ts Outdated Show resolved Hide resolved
src/__tests__/util.test.ts Show resolved Hide resolved
src/workspace-resolver.ts Outdated Show resolved Hide resolved
A bunch of stuff that was (and wasn't) needed for abstracting workspaces.
It's not yet the most efficient, but it passes the test suite!
@lishaduck
Copy link
Contributor Author

lishaduck commented Jul 18, 2024

Almost to a point I'm happy. I've got it reusing the package.json, now I just want to extract out fs to an interface so I can just mock the fs in tests instead of the cod, which should help readability (and code coverage).

And then I'll need to clean up the history a bit more...

src/workspace-resolver.ts Outdated Show resolved Hide resolved
This makes jump-to-definition much more useful.
I tried to enabled recommended-type-checked, but somehow ended up just making the arrays consistent.
Copy link
Owner

@jeffijoe jeffijoe left a comment

Choose a reason for hiding this comment

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

Looks good! Just gotta get CI to pass now.

@lishaduck
Copy link
Contributor Author

Looks good! Just gotta get CI to pass now.

Yup. I'll also need to get some tests (which I suppose is needed for CI).
Can you revert node to 22.4.x? 22.5.x still hasn't had a patch for npm.

@jeffijoe
Copy link
Owner

CI uses current so it'll always pull the latest. Let's give it another day, I'm sure they'll release a patch soon.

@lishaduck
Copy link
Contributor Author

CI uses current so it'll always pull the latest. Let's give it another day, I'm sure they'll release a patch soon.

Ok, fair enough. See ya then!
(I'm working through tests. Almost done (I hope))

@lishaduck
Copy link
Contributor Author

Added some tests, should be 100% coverage, but I don't have a good way of checking.

@jeffijoe
Copy link
Owner

Added some tests, should be 100% coverage, but I don't have a good way of checking.

npm run cover

that should do it

@lishaduck
Copy link
Contributor Author

Screenshot 2024-07-18 at 8 35 03 PM

How is this line untested?

export function createWorkspaceResolverService({

It's just a function declaration. And it's causing branch coverage problems? What?

@lishaduck
Copy link
Contributor Author

Ok, that was weird. I guess parameter defaults get considered as the declaration line, and branching %s factor in how high into the call stack it is???

@jeffijoe
Copy link
Owner

Were you able to get the last branch covered?

Also, looks like the bug has been fixed, we're just waiting for a release.

@lishaduck
Copy link
Contributor Author

lishaduck commented Jul 19, 2024

Were you able to get the last branch covered?

Yup. 1860518

Also, looks like the bug has been fixed, we're just waiting for a release.

Yup. https://nodejs.org/en/blog/release/v22.5.1

Can I get a CI rerun?

@lishaduck
Copy link
Contributor Author

@jeffijoe, ping :)
Can you retrigger ci?

@jeffijoe jeffijoe merged commit 6d093fa into jeffijoe:master Jul 22, 2024
5 checks passed
@jeffijoe
Copy link
Owner

Thanks! I’ll release this tomorrow. :) nice work!

@lishaduck
Copy link
Contributor Author

Thanks! I’ll release this tomorrow. :) nice work!

Thanks to you! It was a pleasure to write, nicely architectured (I'll admit I'm not a big fan of dependency injection containers, but they do serve a purpose). It was lots of fun, entirely worthwhile, and feel free to ask me to fix monorepo support again if it breaks and you're not in mood to fix it yourself becuase I actually want to find an excuse to write some more code here 😉1

Footnotes

  1. And, of course, it'll come at the wrong time and I'll regret saying so. But for now, feel free. 😜

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.

Support PNPM Monorepos
3 participants