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

Update packages #49

Merged
merged 3 commits into from
Jun 7, 2024
Merged

Update packages #49

merged 3 commits into from
Jun 7, 2024

Conversation

whilefoo
Copy link
Contributor

@whilefoo whilefoo commented Jun 4, 2024

Some packages were outdated and by upgrading the packages the types have changed so I had to fix that

0x4007
0x4007 previously approved these changes Jun 5, 2024
@0x4007 0x4007 dismissed their stale review June 5, 2024 01:59

Just realized types are broken in CI. So this pull isn't ready.

Copy link
Member

@gentlementlegen gentlementlegen left a comment

Choose a reason for hiding this comment

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

I am fine with the update. Out of curiosity, why do we never pin packages to a specific version?

@whilefoo
Copy link
Contributor Author

whilefoo commented Jun 5, 2024

tests/main.test.ts(139,18): error TS2339: Property 'incentives' does not exist on type '{ plugins: { status: { name?: string | undefined; description?: string | undefined; command?: string | undefined; example?: string | undefined; uses: { id?: string | undefined; type: "github"; plugin: string | GithubPlugin; with: { ...; }; }[]; skipBotEvents: boolean; }[]; ... 301 more ...; "*": { ...; }[]; }; }'.

@gentlementlegen this test doesn't make sense to make because the config is not correct for the kernel, maybe you meant that as plugin config?

Out of curiosity, why do we never pin packages to a specific version?

I'm not sure, but we probably should. What ends up happening is that we have ^12.1.0 in the package.json but yarn install will install 12.6.0 which might break something even though it shouldn't according to the convention.

@gentlementlegen
Copy link
Member

@whilefoo I think this test was done before we updated the config, so it was using the old bot config and was valid. I didn't spot it since the test still works due to the mock, I should be changed indeed.

I also agree with that, we should definitely set packages to a specific version moving forward.

@rndquu
Copy link
Member

rndquu commented Jun 7, 2024

Regarding this CI run

Can we simply:

  1. @ts-ignore this line since Expression produces a union type that is too complex to represent. #31 has already taken too much dev effort for a single tsc error
  2. Remove this line if incentives is no longer present in the kernel's config

@whilefoo whilefoo merged commit 4169a83 into development Jun 7, 2024
5 of 6 checks passed
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