RocketChat / RC4Community

Full-stack components for building, engaging, and growing your massive on-line community
https://community.rocket.chat/
Apache License 2.0
48 stars 68 forks source link

[IMP][WIP] Migrate Strapi to v4 #162

Closed Dnouv closed 2 years ago

Dnouv commented 2 years ago

This PR migrates the current Strapi v3 to v4. Currently known issues:

Things need to be done:

Thank you!

PS. The Strapi v4 works with yarn only. That's why there's a yarn.lock. Instead of npm install do yarn install Instead of npm run build do yarn build

Fixes: #161

lgtm-com[bot] commented 2 years ago

This pull request introduces 2 alerts when merging 9f59c53a308689451d86713a441b1869fd39fe73 into 6dd0812f286e9922770da0279b68ba5ff22671f9 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 2 alerts when merging c9129dd830b19d8c143e6278d838d434f121772b into 6dd0812f286e9922770da0279b68ba5ff22671f9 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 2 alerts when merging 09fec80b03bcb267202a43e14a02453ed6cd398d into 6dd0812f286e9922770da0279b68ba5ff22671f9 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 2 years ago

This pull request introduces 2 alerts when merging 936b80dbb373cf24402ea37041373ca6dbb3fa86 into 6dd0812f286e9922770da0279b68ba5ff22671f9 - view on LGTM.com

new alerts:

Sing-Li commented 2 years ago

@Dnouv build check is failing. Can you please check what the problem might be. Thanks.

image

Appears to be a check on existence of package-lock.json @debdutdeb is this necessary anymore?

Dnouv commented 2 years ago

Thanks for the review @Sing-Li yes, I have checked actually the build process expects a package-lock but in this PR we have the yarn.lock, so it is failing due to this missing file.

Sing-Li commented 2 years ago

@Dnouv if you have switched to yarn - the README.md(s) need to be updated? Currently they all use npm i and so on.

Sing-Li commented 2 years ago

@Dnouv also with yarn can volta still work to script version lock (to improve dev ex out of box?)

I'm also struggling with the version of yarn that is required --- you may want to mention this in the README.md(s)

Dnouv commented 2 years ago

My bad, I will update the README, we only need the yarn in cms, the version which was currently in use with volta is 1.22.18.

Sing-Li commented 2 years ago

Seems the sane way to proceed from a blank checkout is :

curl --compressed -o- -L https://yarnpkg.com/install.sh | bash

then

volta install node@16

Assuming that strapi4 works with the latest node LTS

Dnouv commented 2 years ago

Yeah, it works with the latest nodeV16🎉, but still have issues running npm 😅

Sing-Li commented 2 years ago

INITIALIZE_DATA=true yarn run develop

ends up with this error currently ....

image

Perhaps you checked in the sqlite db with some pre-populated (and later duplicated) data?

debdutdeb commented 2 years ago

Ok, so no we don't need that check. BUT, I'm confused here. Have we switched totally to yarn? If yes why? Or is it yarn for cms, npm for app. Which is a design change I dislike and don't think is a good idea to entertain.

Dnouv commented 2 years ago

The server is starting properly on the local setup. Maybe we could try deleting the old .tmp and build files? @Sing-Li

Dnouv commented 2 years ago

Ok, so no we don't need that check. BUT, I'm confused here. Have we switched totally to yarn? If yes why? Or is it yarn for cms, npm for app. Which is a design change I dislike and don't think is a good idea to entertain.

There was an issue that is still there but was marked as resolved on the Strapi GitHub page here. And we also hit that. Currently, the yarn is only for cms. Thank you!

Sing-Li commented 2 years ago

Why only cms? @Dnouv

If we switch to yarn - let's do it across project. Otherwise, we add tooling baggage for a very poor developer experience.

Sing-Li commented 2 years ago

@Dnouv after deleting .tmp and build (please add to README.md) I've got this error on a clean checkout....

image

debdutdeb commented 2 years ago

@Dnouv

As @Sing-Li said, if you must move to yarn, make it project wide, not like this.

Secondly, problems like this, arise from time to time at different scale in any kind of project, be it big or small when you depend on something external. From a quick sweep over the linked issue, I could see that moving to yarn is a workaround. Changing something like your package management system and a component of your build system, project wide, is not a sensible approach just because there is a bump. I'm not saying moving to yarn is a bad idea. But for the reason you're doing it, is wrong.

A better approach is to update the README to document an appropriate workaround or include a helper, i.e. doing it in a non critical way, and wait for the issue to be fixed upstream and then updating our project to reflect that.

If you want to move to yarn, sure. There are many advantages to yarn over npm, but please make sure the reason you're doing it is not flickery, because what if tomorrow the same problem surfaces with yarn but npm works fine?

Dnouv commented 2 years ago

Thanks for the heads up @debdutdeb True, makes sense if we'd do like then we would probably in the worst-case scenario need to make shifts for any and all packages update. I will take care of it next time, my apologies. Although the 4.1.10 seems to be working fine with npm. Maybe we could add a Note below stating if you run into these specific issues, try out this workaround (yarn)

Sing-Li commented 2 years ago

Success! Congrats. This is a monumental job @Dnouv (and helpers!)

Our dev team regularly does such extensive migration / refactor in production. But usually it is a team effort and takes a significant amount of time and full of many more diverting opinions and discussions.

Kudos! Merging it now.

image

sidmohanty11 commented 2 years ago

@Dnouv really amazing work!