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

feat: move to pnpm #2652

Merged
merged 6 commits into from
Feb 13, 2025
Merged

feat: move to pnpm #2652

merged 6 commits into from
Feb 13, 2025

Conversation

capJavert
Copy link
Contributor

@capJavert capJavert commented Feb 11, 2025

  • also update to node 22

@capJavert capJavert self-assigned this Feb 11, 2025
@capJavert capJavert force-pushed the feat-pnpm branch 2 times, most recently from 483929a to 848795b Compare February 11, 2025 18:49
Comment on lines +118 to +123
declare module 'fs' {
interface ReadStream {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
on(event: 'data', listener: (chunk: any) => void): this;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In node 22.x types they changed this so its always Buffer so you can't cast it, and since typeorm uses it for query.stream() I overwritten it here to any so we can type it as usual until Node supports generics here.

@@ -389,7 +389,7 @@ export const schedulePersonalizedDigestSubscriptions = async ({

reject(error);
});
personalizedDigestStream.on('end', resolve);
personalizedDigestStream.on('end', () => resolve(true));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In node 22.x types signature of resolve requires value, so just updating

"test": "jest --testEnvironment=node --runInBand",
"typeorm": "typeorm-ts-node-commonjs",
"postinstall": "patch-package"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pnpm has its own patch command

.nvmrc Show resolved Hide resolved
.infra/package.json Outdated Show resolved Hide resolved
@capJavert capJavert marked this pull request as ready for review February 12, 2025 00:25
@capJavert capJavert requested a review from a team as a code owner February 12, 2025 00:25
@@ -6,19 +6,21 @@ orbs:
jobs:
build:
docker:
- image: cimg/node:20.12
- image: cimg/node:22.13
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do 22.11 as apps?
Or move apps over to this as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

between 22.11 and 22.13 there is no breaking changes, so either way, we can leave everything as is I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes more sense to update apps if we really wanted to

Copy link
Contributor

Choose a reason for hiding this comment

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

I just like stuff being the same 😂
Yeah no worries I'll do apps later.

@@ -1 +1 @@
20.12
22.13
Copy link
Contributor

Choose a reason for hiding this comment

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

Does nvm file not propogate down? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh you mean because it exists in root, I am not sure, but it was here before so just left it

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah because of root one, and they match.

@capJavert capJavert merged commit 6176bf0 into main Feb 13, 2025
8 checks passed
@capJavert capJavert deleted the feat-pnpm branch February 13, 2025 08:29
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