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

chore(package.json): Update dependencies and node version #727

Merged
merged 16 commits into from
Jun 11, 2024
Merged

Conversation

1p22geo
Copy link
Contributor

@1p22geo 1p22geo commented May 31, 2024

What kind of change does this PR introduce?

  • Use node v20.9.0+ (or anything > 20.9.0) as specified in README instead of stricly requiring node == 20.0.0 (as it was in package.json)
  • Update to use latest yarn
  • Yarn is a package manager and not a dependency, it is not needed in runtime. Yarn installs the dependencies, so no need to mention it in dependencies in package.json

Issue Number:
There weren't any, as it is not technically a bug. Yet.

Screenshots/videos:

Sorry, none here.

If relevant, did you update the documentation?

Not relevant.

Summary

why upgrade yarn

I know that package.json updates are stricly forbidden for this repository (there is even a CI workflow to fail PRs which changed it) but this is kinda important.

I know it will cause major pain, especially for CI runs, but I can help with migrating workflows to yarn 4.

Does this PR introduce a breaking change?

Yes.

1p22geo added 2 commits May 31, 2024 15:14
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.
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.
@1p22geo 1p22geo requested a review from a team as a code owner May 31, 2024 13:49
@1p22geo
Copy link
Contributor Author

1p22geo commented May 31, 2024

Hold on, I am fixing the build errors...

@1p22geo
Copy link
Contributor Author

1p22geo commented May 31, 2024

Builds and CI succeed now, and the only thing failing is the unauthorized file edit workflow. All workflows and builds use latest yarn and node now.

@1p22geo 1p22geo changed the title chore: Update dependencies and node version chore(package.json): Update dependencies and node version Jun 2, 2024
@benjagm
Copy link
Collaborator

benjagm commented Jun 6, 2024

Thanks a lot for this PR! We'll need some extra time to review it but hope to provide feedback tomorrow the latest.

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 6, 2024

Sorry for the tons of unnecessary updates here, I just wanted to fix conflicts with upstream (after merging #729 and #730 which also updated package.json and yarn.lock).

package.json Outdated Show resolved Hide resolved
Copy link
Member

@aialok aialok left a comment

Choose a reason for hiding this comment

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

See comment. Apart from the comment (LGTM).

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 7, 2024

  • No more yarn workspaces focus, everywhere yarn install --frozen-lockfile
  • node ^20.9.0 in package.json
  • node 20.9.0 in github actions

@1p22geo 1p22geo requested a review from aialok June 7, 2024 09:16
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@aialok
Copy link
Member

aialok commented Jun 7, 2024

@1p22geo Please make sure you always fetch latest upstream as this is going to be major changes in our codebase.

Thank you : )

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 7, 2024

I see the errors, fixing now...

Copy link
Member

@aialok aialok left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Warning: This PR might introduce breaking changes in production, as there are complete changes in our yarn.lock file and yarn is now in an upgraded version.

Thank you @1p22geo : )

@benjagm
Copy link
Collaborator

benjagm commented Jun 7, 2024

Hi @1p22geo do you mind if we change the target branch to try the deployment first in cloudflare? Once that works well merge the temorary branch to main.

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 7, 2024

Hi @1p22geo do you mind if we change the target branch to try the deployment first in cloudflare? Once that works well merge the temorary branch to main.

Of course, I've only tried it with latest node. Once any errors occur, I'd be happy to help fix.

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 11, 2024

Updated to upstream and resolved conflicts.

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 11, 2024

BTW may I know how is the whole "temporary deployment" thing going? Are there any errors that need to be fixed, or is it just difficult to set up?

@benjagm benjagm changed the base branch from main to web-node-20-9-0 June 11, 2024 11:42
Copy link
Collaborator

@benjagm benjagm left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@benjagm benjagm merged commit df5fafb into json-schema-org:web-node-20-9-0 Jun 11, 2024
5 of 6 checks passed
@benjagm
Copy link
Collaborator

benjagm commented Jun 11, 2024

BTW may I know how is the whole "temporary deployment" thing going? Are there any errors that need to be fixed, or is it just difficult to set up?

We have been busy!! Merged!

@benjagm
Copy link
Collaborator

benjagm commented Jun 11, 2024

I tested everything in cloudflare and I needed to add yarn back as dependency because we are using it to generate the sitemap files at deployment time. You can see the content fo the branch here: https://github.com/json-schema-org/website/tree/web-node-20-9-0

This is the build command we are running in cloudflare : next build && next-sitemap --config next-sitemap.config.js

Now everything is working in web-node-20-9-0 and we are ready to merge to main.

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 11, 2024

Why is it yarn 1.22 again? I added Yarn 4...

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 11, 2024

packageManager is yarn 4.2 (latest)

"packageManager": "[email protected]"

but you added yarn 1.22?
"yarn": "1.22.22",

@benjagm
Copy link
Collaborator

benjagm commented Jun 11, 2024

Last version of yarn is 1.22

https://github.com/yarnpkg/yarn/releases

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 11, 2024

image
Then am I a time traveller?

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 11, 2024

I literally gave you this link in the PR description https://yarnpkg.com/migration/overview

@benjagm
Copy link
Collaborator

benjagm commented Jun 11, 2024

:-) I am not an expert. I am just trying to keep the website running while managing the community

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 11, 2024

Also, it's the stale repo.
https://github.com/yarnpkg/berry

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 11, 2024

Ah wait, somehow yarn@latest is yarn 1.22, but yarn@stable is yarn 4.2
npm install yarn or yarn add yarn will add yarn 1.22 (default)
yarn add yarn@stable or yarn set version stable will install ACTUAL latest

@benjagm
Copy link
Collaborator

benjagm commented Jun 11, 2024

It seems than the last yarn version available as dependency is the 1.22.22 version:

@1p22geo
Copy link
Contributor Author

1p22geo commented Jun 11, 2024

Wait, you said you run next build && next-sitemap --config next-sitemap.config.js on the Cloudflare worker.

But next is not a regular binary file. To run it you need either yarn or npx or a package.json script. Which means that yarn is installed already, before running anything. (or some other package manager)

yarn is the thing that's doing the installing, so it shouldn't really be installing itself.

Also you can run yarn set version stable to install yarn v4

Did it really not work without yarn in the dependencies?

@benjagm
Copy link
Collaborator

benjagm commented Jun 11, 2024

Hi again @1p22geo. We are not using cloudflare workers, we are using cloudflare pages. If we remove yarn from the dependencies this is the error we get in the cloudflare deployment:


13:45:10.141 | Verify run directory
-- | --
13:45:10.142 | Executing user command: next build && next-sitemap --config next-sitemap.config.js
13:45:10.145 | /opt/build/bin/build: line 39: next: command not found
13:45:10.149 | Failed: build command exited with code: 127
13:45:11.023 | Failed: error occurred while running build command

As soon as I add yarn to the dependencies it works like a charm.

I'd like to clarify that we are not expert frontend developers, we are a group o people passionate about JSON Schema, the Specification and its tooling. These type of changes that maybe are trivial for you, may be more complex for us. Our expertise field is the spec, JSON Schemas design and processing and developer tooling .... so please bear with us.

@jdesrosiers
Copy link
Member

I agree that it doesn't make sense for yarn to be a dependency. Ideally you would call yarn run build instead of next build && next-sitemap --config next-sitemap.config.js in Cloudflare Pages. It runs the same commands, but does so in the context of the installed dependencies (like next and next-sitemap). @benjagm, can we give that a try on Cloudflare Pages without yarn in the dependencies and see if that works?

@jdesrosiers
Copy link
Member

@benjagm I remembered that I have access the Cloudflare Pages, so I tried it myself. I changed the command to yarn build and redeployed the branch without the yarn dependency. Everything deployed fine, but it seems Cloudflare Pages decided to use yarn 1.22.4 rather than 4.2.2 as declared in package.json.

17:28:44.100 | Installing yarn at version 1.22.4

It seems that it doesn't respect the packageManager field. Not sure if there's a way around that problem, but I'd say that's a blocker for upgrading to yarn v4 if that problem can't be solved.

After my testing, I changed all the settings back to how they were before. I'll let you decide whether or not to make the change permanent.

@jdesrosiers
Copy link
Member

So, I found out that Cloudflare Pages has v1 and v2 build systems. The v2 build system respects packageManager, but we were using v1. I did another test deployment, this time using the v2 build system and yarn bulid as the build command. Everything seems to have worked and it's using the right version of yarn this time.

17:49:54.036 | Preparing [email protected] for immediate activation...

@benjagm, I reverted all the Cloudflare configuration changes I made so you have a chance to do more thorough testing (I did little more than make sure it built without errors) before making the changes permanent.

@jdesrosiers
Copy link
Member

@1p22geo I want to thank you for taking the time to do this upgrade. Keeping dependencies up-to-date is a very important security practice.

That said, I want to call your attention to an interaction that occurred in this thread.

Then am I a time traveller?

I literally gave you this link in the PR description https://yarnpkg.com/migration/overview

Some of us found these comments to be disrespectful. I know cultural differences as well as language barriers can lead to misunderstandings especially in async written communications like this, but I wanted you to know this was how it was received and I hope you'll adjust as I'm sure you don't mean to offend anyone. @benjagm might not be a web development expert, but this website and many other things in this community wouldn't exist without his efforts. He deserves our respect.

Thanks again for your contributions and I'm glad to have you as part of this community.

@benjagm
Copy link
Collaborator

benjagm commented Jun 12, 2024

Thanks everyone! @jdesrosiers Good finding with the v1 vs v2 build system of cloudflare. I changed it and it starts using the modern Yarn. That works.

I'd like to recall that we are doing all of this use site-map to generate the sitemap xml files and improve our SEO. These are the scripts we have configured in package.json:

  "scripts": {
    "build": "next build",
    "postbuild": "next-sitemap --config next-sitemap.config.js",

I have checked the result of using yarn run build in cloudflare and unfortunately it doesn't the postbuild script necessary to generate the sitemap. After doing some research i found this question in their forum and I ended up using yarn run build && yarn run postbuild and now everything works as expected:

  • Yarn removed from dependencies
  • Yarn as package manager with V 4.2.2
  • Sitemap generate properly in cloudflare

This is the new PR to master branch: #757

@jdesrosiers
Copy link
Member

I'm surprised that postbuild doesn't run automatically when you run build. I thought that was its purpose. Anyway, that's why I left it for you to verify things were working 😄.

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.

5 participants