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

Refactor into a pnpm workspace #89

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

Conversation

jakebailey
Copy link
Member

Fixes #81

Outstanding questions:

  • Should it really do build-all on prepare?
  • Should site be a part of the monorepo?
    • If so, should build-all build the site?
  • Should I add a corepack version specifier for pnpm, or will people be able to figure out how to install pnpm themselves?
  • Would you benefit from something that helps bump the version numbers in the examples? I'm not sure what this would be other than something like changesets.

Theoretically I can attempt to do this via plain npm though @DanielRosenwasser had problems with it hence asking me to send this PR for real.

- run: npm run build-all
cache: 'pnpm'

- run: pnpm install
Copy link
Member Author

Choose a reason for hiding this comment

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

This awkwardly already runs build-all.

@DanielRosenwasser
Copy link
Member

Should it really do build-all on prepare?

I'd prefer it didn't, but our docs would need to be updated if that changes.

Should site be a part of the monorepo?

No, I want that to stay out of everyone's way.

Should I add a corepack version specifier for pnpm, or will people be able to figure out how to install pnpm themselves?

I don't think it makes much of a difference, right? If we did this, would it work out of the box for anyone with Node 18+? Or would people have to run a command?

Would you benefit from something that helps bump the version numbers in the examples? I'm not sure what this would be other than something like changesets.

I can just run that manually I think. It doesn't need to be a script.

@jakebailey
Copy link
Member Author

No, I want that to stay out of everyone's way.

Do you want me to convert that to pnpm just so it's not mixed package managers?

I don't think it makes much of a difference, right? If we did this, would it work out of the box for anyone with Node 18+? Or would people have to run a command?

People still have to run corepack enable, it's just that you will only be using one version of pnpm, which I'd argue is better (this is how I have it set up on TS).

Would you benefit from something that helps bump the version numbers in the examples? I'm not sure what this would be other than something like changesets.

I can just run that manually I think. It doesn't need to be a script.

Right now there's no script or anything; you can technically leave the old specifiers in but pnpm won't help you update them everywhere as far as I know.

npm run build
git clone https://github.com/microsoft/TypeChat
cd TypeChat
pnpm install
Copy link

Choose a reason for hiding this comment

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

Not everyone knows pnpm, I'd recommend adding pnpm install instructions (it could seem like a typo). https://pnpm.io/cli/install

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.

all samples package.json do not have typechat as a dependency
3 participants