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

Modernize stack and convert to vue+vite #68

Merged
merged 10 commits into from
Oct 18, 2023

Conversation

davidglezz
Copy link
Contributor

@davidglezz davidglezz commented Sep 23, 2023

Questions Answers
Description? There are a lot of outdated deps. This PR migrates from webpack 1 to vite and from React+jQuery to Vue.
Type? refactor
BC breaks? yes
Deprecations? n/a
Fixed ticket? Fixes #65
Sponsor company
How to test? Manual compare with https://translators.prestashop-project.org/
  • From old Webpack 1 to Vite.
  • From node 14 to node 20.
  • From old js React+jQuery to Vue.
    • For now, its a simple manual component conversion. In the future it could be restructured and refactored.
    • The final bundle size is reduced from 387KB to 71KB.
  • Removed a lot of old dependencies.
  • It still using Bootstrap 4 alpha, maybe it's better to do a small redesign than try to make it look the same with Bootstrap 5

Why change to Vite and Vue instead of fixing the dependency problem that existed?

Really old libraries were used, Webpack 1 and a lot of loaders. Due to the breaking changes of new versions of webpack and loaders, it would take me more time than just starting a vue project and migrating a dozen components.

Signed-off-by: davidglezz <davidg@empathy.co>
Signed-off-by: davidglezz <davidg@empathy.co>
@davidglezz davidglezz changed the title feat: convert to vue+vite feat: modernize stack and convert to vue+vite Sep 23, 2023
…the same name."

Signed-off-by: davidglezz <davidg@empathy.co>
Signed-off-by: davidglezz <davidg@empathy.co>
Signed-off-by: davidglezz <davidg@empathy.co>
Signed-off-by: davidglezz <davidg@empathy.co>
Signed-off-by: davidglezz <davidg@empathy.co>
Signed-off-by: davidglezz <davidg@empathy.co>
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Hi @davidglezz

I will try to review this PR in next days, it's a big one 😄

Really old libraries were used, Webpack 1 and a lot of loaders. Due to the breaking changes of new versions of webpack and loaders, it would take me more time than just starting a vue project and migrating a dozen components.

I agree 👍 code is so old we can simply say we start over from an almost blank page.

Question: don't you think that even Vue or Vite are over-engineered for this site?

When I look at the result I feel like we could almost do it in vanilla JS and using only Bootstrap 😄 . And render the site on server side once, and serve it as static HTML files instead of computing the end result into browser.

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

This PR needs to run Node 20 to be tested

@kpodemski
Copy link

wow @davidglezz !

@davidglezz
Copy link
Contributor Author

Thanks
This week I am without laptop because si am on vacations. See you next week 😁

@matks
Copy link
Contributor

matks commented Sep 27, 2023

Before I can test this PR I need to install Node.js 20 on my laptop without breaking my stack 😅 might take a few time

Copy link

@matthieu-rolland matthieu-rolland left a comment

Choose a reason for hiding this comment

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

what a PR !

how do we proceed, I guess it should not be tested by QA ?

(@matks if you have nvm installed, you can change node version without breaking anything 👍 )

@davidglezz
Copy link
Contributor Author

@matks

Question: don't you think that even Vue or Vite are over-engineered for this site?

Probably yes, a bit, Vue is mainly used as template engine and is the easiest path (for me) to migrate from old react codebase.

Converting to framework-less will require more effort IMO.

@davidglezz
Copy link
Contributor Author

davidglezz commented Oct 3, 2023

I see other issues:

  • Crowdin data is transformed several times, from json to csv to json and finally the js object is mapped to another. This should be simplified.
  • Not all languages are rendered/listed. There are 82 languages listed in crowdin vs 67 in statistics.json

@matks
Copy link
Contributor

matks commented Oct 4, 2023

Oh yeah there are a loooooot of issues with this repository 😛

@mflasquin
Copy link

Before I can test this PR I need to install Node.js 20 on my laptop without breaking my stack 😅 might take a few time

https://github.com/nvm-sh/nvm 😉

@matks
Copy link
Contributor

matks commented Oct 6, 2023

And here we go 😄 troubleshooting brew updates dyld: Library not loaded: /usr/local/opt/icu4c/lib/libicui18n.72.dylib

EDIT: argh 😭 so many deps to update

Signed-off-by: davidglezz <davidg@empathy.co>
Signed-off-by: davidglezz <davidg@empathy.co>
@matks
Copy link
Contributor

matks commented Oct 16, 2023

For the record I broke my php + node setup on my laptop 😛 when trying to install Node 20. This is because of https://www.skymac.org/Admin-Dev/article-5038c427-Homebrew-Corriger-un-probleme-de-dependances-trop-recentes.htm

I think I will reinstall my laptop from zero 😄 too many piles of software on it and now it all crumbled.

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Hello @davidglezz so today I did the full reinstall of my environment 😄 it's all clean and shiny now! I also installed nvm to be able to easily switch Node versions.

I attempted to test your PR but I have this issue: when I browse front/index.html I get console error

Failed to load module script: Expected a JavaScript module script but the server responded with a MIME type of "video/mp2t". Strict MIME type checking is enforced for module scripts per HTML spec.

Here is what I did:

  1. Fetch your branch
  2. cd front
  3. npm install using npm 10.1.0
  4. npm run build using node 20.8.1
  5. Browse index.html

@davidglezz
Copy link
Contributor Author

davidglezz commented Oct 18, 2023

@matks I'm glad you renewed the node setup successfully.

You can do npm run dev (dev build) or
npm run build && npm run preview (to preview production build - recommended) or
npm run build and serve with other tool the ../public (not front/index.html)

And thank you very much for taking the time to review my PR.

@davidglezz
Copy link
Contributor Author

There are things that are a bit ugly, but that's because I did this in a few hours, I didn't want to spend more time doing some refactoring or improvements without being sure that I'm on the right track and knowing that it could be merge.

Feel free to request any changes or ask me anything you want.

Have a nice day =)

@matks
Copy link
Contributor

matks commented Oct 18, 2023

@matks I'm glad you renewed the node setup successfully.

You can do npm run dev (dev build) or npm run build && npm run preview (to preview production build - recommended) or npm run build and serve with other tool the ../public (not front/index.html)

And thank you very much for taking the time to review my PR.

I did npm run build and serve with other tool the ../public because if I am not wrong this is what the real env will look like.

It works on my machine 🎉 so I think code is "good enough" for now 😄 . This PR is already big, improvements should be done in other PRs.

Now how do we plug this into existing GitHub Action pipeline and test it?

What we could do is create a new branch on this repository, a branch refacto, and push updated workflow on this branch. I think I can configure GitHub to build the refacto branch on a Environment, allowing us to verify the result. Wdyt?

@davidglezz
Copy link
Contributor Author

I think it's great!

@matks matks mentioned this pull request Oct 18, 2023
@matks matks changed the base branch from prod to refacto October 18, 2023 20:13
@matks matks changed the title feat: modernize stack and convert to vue+vite Modernize stack and convert to vue+vite Oct 18, 2023
@matks
Copy link
Contributor

matks commented Oct 18, 2023

Branch refacto is created. Let's merge this PR into refacto and then I will build a test environment for GitHub Actions from refacto. I also added the hacktoberfest label to the repository 😉 so if you participate in Hacktoberfest this PR will be eligible

@matks matks merged commit 11ed6ed into PrestaShop:refacto Oct 18, 2023
@matks
Copy link
Contributor

matks commented Oct 18, 2023

Env configured 325ec08

@matks
Copy link
Contributor

matks commented Oct 18, 2023

Let's see if it works #70

@matks matks mentioned this pull request Oct 18, 2023
@matks
Copy link
Contributor

matks commented Oct 18, 2023

Everything looks good -> #71 PR to deploy it to prod

If OK for you @davidglezz then tomorrow we can deploy in production

@davidglezz
Copy link
Contributor Author

Perfect!

matks added a commit that referenced this pull request Oct 19, 2023
@matks
Copy link
Contributor

matks commented Oct 19, 2023

@davidglezz Your changes are live.

If you're motivated, there are warnings & notices in the logs https://github.com/PrestaShop/TopTranslators/actions/runs/6571132956/job/17849715207#step:12:1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Degraded Mode
5 participants