Skip to content

Conversation

dario-piotrowicz
Copy link
Contributor

No description provided.

Copy link

pkg-pr-new bot commented Oct 9, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/aws@536

commit: 5d7299d

@dario-piotrowicz
Copy link
Contributor Author

dario-piotrowicz commented Oct 9, 2024

Hey @conico974 👋

@vicb mentioned that you were interested in exploring the use of pkg.pr.new? 🙂

I figured I could start the effort and get the ball rolling ⚽

Please have a look, also do you know why the gh validation is failing? I don't think my branch includes any changes that should make those fail 😕

- name: Check codestyle
run: pnpm lint
- name: Build package
run: pnpm -F @opennextjs/aws build
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this could be pnpm build if preferred, I set it to only build the aws package as the build script was failing in ci for some reason 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably related to the comment i left below about pnpm

Copy link
Contributor

@conico974 conico974 left a comment

Choose a reason for hiding this comment

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

Did you run pnpm install with pnpm 9 ? Someone had done this when we moved the repo and it caused a ton of issues (including the build failing).
Not sure why it cause an issue (and i haven't took the time to look at it), but reverting to pnpm 8 fixed the issue.
We'll need to figure this out but it might have something to do with a big change in the lockfile version between pnpm 8 and 9

on:
pull_request:
branches:
- main
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to filter to run this only on change on the packages/open-next folder itself.
Really not a big deal but change made to e2e would trigger this which is not really useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point! 👍

@dario-piotrowicz
Copy link
Contributor Author

Did you run pnpm install with pnpm 9 ? Someone had done this when we moved the repo and it caused a ton of issues (including the build failing). Not sure why it cause an issue (and i haven't took the time to look at it), but reverting to pnpm 8 fixed the issue. We'll need to figure this out but it might have something to do with a big change in the lockfile version between pnpm 8 and 9

Awesome! thanks yeah I tried with pnpm 9, let's give pnpm 8 a go 🙂

@dario-piotrowicz
Copy link
Contributor Author

@conico974 unfortunately using pnpm 8 didn't seem to help 🥲

@conico974
Copy link
Contributor

Yeah not sure what's going on here.
This https://github.com/opennextjs/opennextjs-aws/actions/runs/11234182017 which run yesterday had absolutely no problem on those file.
But somehow it fails today on those same file (and that's not file you touched in your PR)

My guess is that there is some kind of issue with the update in the lockfile.
It seems to have updated a ton of dependencies and one of them is probably causing some kind of issue.

I'll try to take another look at this tomorrow

@conico974
Copy link
Contributor

Same issue as what happened on this PR #529

@dario-piotrowicz
Copy link
Contributor Author

dario-piotrowicz commented Oct 9, 2024

@conico974 all better now 🫡 (PS: thanks a bunch for the support! 🫶)

I basically restarted the whole thing from scratch, but starting using pnpm 8 from the start now and as you can see the lock file is much much much smaller... I guess pnpm was getting confused by the dependencies or something 😕 (even if I did try deleting the lock file and clearing the node_modules 🤷)

Copy link
Contributor

@conico974 conico974 left a comment

Choose a reason for hiding this comment

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

Great, that's perfect.
We'll need to figure out what's going on with this lockfile, there is something very wrong here.
Anyway thanks for the PR

@conico974 conico974 merged commit 47471a7 into main Oct 9, 2024
1 check passed
@conico974 conico974 deleted the dario/pkg-pr-new branch October 9, 2024 23:07
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.

2 participants