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.

Sign up for GitHub

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

Jump to bottom

Continuation of PR 727 - chore(package.json): Update dependencies and node version #757

Closed
wants to merge 7 commits into from
Closed

Continuation of PR 727 - chore(package.json): Update dependencies and node version #757

wants to merge 7 commits into from

Conversation

benjagm
Copy link
Collaborator

@benjagm benjagm commented Jun 12, 2024

The PR #727 was merged into web-node-20-9-0 branch to make some test of the node and yarn version in cloudflare.

Now that everything works this PR is intended to promote the change to the main branch.

1p22geo and others added 5 commits June 11, 2024 13:44
@1p22geo @aialok
chore(package.json): Update dependencies and node version (#727)
df5fafb 
* patch!: use the version of node in the documentation

So, hear me out. Inside README.md you claim that the project needs node v20.9.0+.
But package.json specifies that it needs to be EXACTLY node 20.0.0

I updated the field in the package.json to use the correct version of
node. Or range of versions, for that matter.

* chore(yarn): use latest stable yarn

Yarn 4 is so much faster and more efficient than yarn 1, and its
dependency resolution algorythm is more secure.

But I understand if you would like to keep the older versions.

* patch(ci): did you know `actions/setup-node` can do caching for you?

* fix: add `moment-timezone` as a dependency

* ci: update caching accordingly for next builds

* merge: resolved conflicts

* revert: Restored the `yarn install`

* patch(package.json): pin node to `^20.9.0`

* patch: the second yarn install

* fix: pinned node version to 20.9.0 in all github actions

* fix: ah yes I forgot about the `@types`

thanks for that one

Co-authored-by: Alok Gupta <[email protected]>

* fix: removed the next-plausible and updated lockfile

* patch: updated lockfile after upstream update

---------

Co-authored-by: Alok Gupta <[email protected]>
@benjagm
Add yarn back
212fd85
@benjagm
Remove yarn behaviour
674ac5d
@benjagm
Update package.json
b4346c5
@benjagm
Leave yarn classic for now
6df3267
@benjagm benjagm requested a review from a team as a code owner June 12, 2024 05:24
@benjagm benjagm changed the title Continuation of PR #727 Continuation of PR 727 - chore(package.json): Update dependencies and node version Jun 12, 2024
@benjagm benjagm mentioned this pull request Jun 12, 2024
Merged
@cloudflare-workers-and-pages Cloudflare Workers and Pages
Copy link

cloudflare-workers-and-pages bot commented Jun 12, 2024 •
edited
Loading

Deploying website with  Cloudflare Pages  Cloudflare Pages

Latest commit: 10c4a34
Status: ✅  Deploy successful!
Preview URL: https://3434e90d.website-2v2.pages.dev
Branch Preview URL: https://web-node-20-9-0.website-2v2.pages.dev

View logs

jdesrosiers
jdesrosiers reviewed Jun 12, 2024
View reviewed changes
Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

.yarn and .yarnrc.yml shouldn't be checked in. It looks like you have .yarn in .gitignore but for some reason .yarn/releases/yarn-4.2.2.2.cjs is getting checked in.

I notice that there are changes to styles/globals.css in this PR. Is that a mistake? It doesn't seem to have anything to do with this PR.

@benjagm
Copy link
Collaborator Author

benjagm commented Jun 13, 2024 •
edited
Loading

Thanks for the reply Jason. It seems that .yarn/releases/. should be added to the repos as recommended in the yarn docs: https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored

I notice that there are changes to styles/globals.css in this PR. Is that a mistake? It doesn't seem to have anything to do with this PR.

When testing everything I found an unexpected bug with the lens icon of the search buttons in the landing page and this change is required to get it fixed.

@jdesrosiers
Copy link
Member

It seems that .yarn/releases/. should be added to the repos as recommended in the yarn docs

Committing large binarys in git is considered bad practice. Committing external dependencies in your project is considered bad practice. So, I had to look into what was going on here. It looks like yarn has changed how they recommend installing yarn and you ended up using an out-dated approach.

The current recommendation is that corepack is used in conjunction with "packageManager" in package.json to automatically install and use the correct version of yarn. With this approach, corepack manages the binaries and there's nothing for us to check in or ignore (as it should be).

The older way of installing yarn installed the binary to the .yarn/releases folder and recommended that you check it in to git. The .gitignore is still configured to allow you to check in .yarn/releases for backward compatibility with the older approach. The new approach simply doesn't use that directory.

You seem to have ended up using this older install method instead of using corepack. I recommend removing everything you have related to yarn and doing a fresh install using corepack. Do corepack enable to ensure corepack is installed and enabled. Then all you have to do is run yarn install and everything will get installed automatically. You don't even need to do yarn set version 4.2.2 because it will get the right version from the "packageManager" field in package.json. I've done this and can confirm that it works and doesn't include a .yarn/releases folder.

@github-actions GitHub Actions
Copy link

Hello! 👋

This pull request has been automatically marked as stale due to inactivity 😴

It will be closed in 180 days if no further activity occurs. To keep it active, please add a comment with more details.

There can be many reasons why a specific pull request has no activity. The most probable cause is a lack of time, not a lack of interest.

Let us figure out together how to push this pull request forward. Connect with us through our slack channel : https://json-schema.org/slack

Thank you for your patience ❤️

@github-actions github-actions bot added the Status: Stale It's believed that this issue is no longer important to the requestor. label Jul 21, 2024
@aialok aialok mentioned this pull request Aug 3, 2024
Merged
@aialok aialok added Status: In Progress This issue is being worked on, and has someone assigned. and removed Status: Stale It's believed that this issue is no longer important to the requestor. labels Aug 3, 2024
@benjagm
Copy link
Collaborator Author

benjagm commented Aug 7, 2024

Closed in favour of #842

@benjagm benjagm closed this Aug 7, 2024
@benjagm benjagm deleted the web-node-20-9-0 branch September 27, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdesrosiers jdesrosiers jdesrosiers left review comments

Assignees
No one assigned
Labels
Status: In Progress This issue is being worked on, and has someone assigned.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@benjagm @jdesrosiers @aialok @1p22geo