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

Ci pnpm #509

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Ci pnpm #509

wants to merge 1 commit into from

Conversation

mcanueste
Copy link
Contributor

@mcanueste mcanueste commented Jul 8, 2023

Description

Please describe your changes. Be descriptive enough to reduce churn for review process.

Checklist

  • discord username: username#0001
  • Closes #
  • PR must be created for an issue from issues under "In progress" column from our project board.
  • A descriptive and understandable title: The PR title should clearly describe the nature and purpose of the changes. The PR title should be the first thing displayed when the PR is opened. And it should follow the semantic commit rules, and should include the app/package/service name in the title. For example, a title like "docs(@kampus-apps/pano): Add README.md" can be used.
  • Related file selection: Only relevant files should be touched and no other files should be affected.
  • I ran npx turbo run at the root of the repository, and build was successful.
  • I installed the npm packages using npm install --save-exact <package> so my package is pinned to a specific npm version. Leave empty if no package was installed. Leave empty if no package was installed with this PR.

How were these changes tested?

Please describe the tests you did to test the changes you made. Please also specify your test configuration.

NOTE: I will fill out the checklist when the MR is ready. This MR is created for debugging pnpm builds

The build process fails here

Seems like contentlayer is not generating files (or failing). Added next-contentlayer following the guide here in case it helps, but seems like it is not working as well.

A github issue that might be related is here.

@mcanueste mcanueste requested a review from a team as a code owner July 8, 2023 23:23
@vercel
Copy link

vercel bot commented Jul 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kampus-ui ❌ Failed (Inspect) Jul 27, 2023 8:33am

@vercel
Copy link

vercel bot commented Jul 8, 2023

@mcanueste is attempting to deploy a commit to the kamp-us Team on Vercel.

A member of the Team first needs to authorize it.

Comment on lines +18 to +20
"@kampus/kampus": "*",
"@kampus/tailwind": "*",
"@kampus/ui-next": "*",
Copy link
Contributor

Choose a reason for hiding this comment

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

why UI package needs these APPs ?
Package should never need an App from the workspace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the @kampus/kampus and @kampus/ui-next imports exist under apps/ui/ (see here).

these imports were working without issues with npm but they need to be defined as dependencies with pnpm as otherwise the imports will fail (due to how pnpm stores packages under node_modules)

Copy link
Member

Choose a reason for hiding this comment

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

This is fine @Ketcap

apps/ui is our storybook application.
packages/ui is the kampus-ui package.

Comment on lines +53 to +59
"@opentelemetry/api": "1.4.1",
"@opentelemetry/core": "1.13.0",
"@opentelemetry/exporter-trace-otlp-grpc": "0.39.1",
"@opentelemetry/resources": "1.13.0",
"@opentelemetry/sdk-trace-base": "1.13.0",
"@opentelemetry/sdk-trace-node": "1.13.0",
"@opentelemetry/semantic-conventions": "1.13.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reasons ? Maybe we can document 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.

was getting the same error as this issue when building with pnpm: contentlayerdev/contentlayer#506

it can be a temporary solution until the main issue with the dependencies are resolved (assuming the version of the contentlayer is incremented on this repo as well)

@@ -0,0 +1,9 @@
import { withContentlayer } from "next-contentlayer";
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be here, this package should not relay on something it's not using.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this one since the build was failing when building gql app since it depends on some files generated with contentlayer under sozluk-content package.

this of course didn't fix the issue and the actual solution was adding -r flag to pnpm run build command so that it starts building sozluk-content before building gql. will test removing this today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just removed this next-contentlayer package thing, and the build process still works properly with -r flag added to pnpm run build command, until this error.

this error is fixed by adding "noImplicitAny": false to tsconfig.json file, but don't know if this is wanted by the maintainers @Ketcap .

@usirin
Copy link
Member

usirin commented Jul 30, 2023

@mcanueste i am gonna take a look at this asap, sorry it took this long.

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.

3 participants