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

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

Merged
merged 42 commits into from
Jun 2, 2022
Merged

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

merged 42 commits into from
Jun 2, 2022

Conversation

Dnouv
Copy link
Member

@Dnouv Dnouv commented May 26, 2022

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

  • The top-nav-item is breaking (fetchData.js line:225)

Things need to be done:

  • Update the API call endpoints in the frontend part.
  • Make changes to the Bootstrap code, and the fetchData.js.
  • Fix Cron tasks

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
Copy link

lgtm-com bot commented May 27, 2022

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

new alerts:

  • 2 for Unused variable, import, function or class

@Dnouv Dnouv marked this pull request as ready for review May 31, 2022 19:04
@Sing-Li
Copy link
Member

Sing-Li commented Jun 2, 2022

@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
Copy link
Member Author

Dnouv commented Jun 2, 2022

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
Copy link
Member

Sing-Li commented Jun 2, 2022

@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
Copy link
Member

Sing-Li commented Jun 2, 2022

@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
Copy link
Member Author

Dnouv commented Jun 2, 2022

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
Copy link
Member

Sing-Li commented Jun 2, 2022

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
Copy link
Member Author

Dnouv commented Jun 2, 2022

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

@Sing-Li
Copy link
Member

Sing-Li commented Jun 2, 2022

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?

Copy link
Member

@Sing-Li Sing-Li left a comment

Choose a reason for hiding this comment

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

pls correct and update

@debdutdeb
Copy link
Member

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
Copy link
Member Author

Dnouv commented Jun 2, 2022

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

@Dnouv
Copy link
Member Author

Dnouv commented Jun 2, 2022

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
Copy link
Member

Sing-Li commented Jun 2, 2022

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
Copy link
Member

Sing-Li commented Jun 2, 2022

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

image

@debdutdeb
Copy link
Member

debdutdeb commented Jun 2, 2022

@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
Copy link
Member Author

Dnouv commented Jun 2, 2022

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)

cms/README.md Show resolved Hide resolved
@Sing-Li
Copy link
Member

Sing-Li commented Jun 2, 2022

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

@Sing-Li Sing-Li merged commit a30b2ae into RocketChat:master Jun 2, 2022
@sidmohanty11
Copy link
Contributor

@Dnouv really amazing work!

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.

[TO DO] Migrate to Strapi 4
4 participants