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

Move frontend to Vite 5 #2775

Merged
merged 18 commits into from
Dec 10, 2023
Merged

Move frontend to Vite 5 #2775

merged 18 commits into from
Dec 10, 2023

Conversation

u5r0
Copy link
Contributor

@u5r0 u5r0 commented Nov 30, 2023 •
edited

Changes:

Tested

  • Storybook tests: on bar with current setup

Not tested:

  • docker config
  • chromatic and graphql codegen commands

Not working

  • Storybook nyc coverage report

related: #2156

@charlesBochet
Copy link
Member

@u5r0 Thank you! We will need to fix the missing pieces before merging it. Especially the typescript part :) I can give it a shot too and see if I can fix that, or maybe other contributors could give a hand!

Quick question, why are you removing the icons?

@u5r0
Copy link
Contributor Author

u5r0 commented Nov 30, 2023

No reason, they're unused yet and seemed in the way.

Is there a preference for the testing setup? Does the server/ jest setup have a say in how the front/ should be?

@charlesBochet
Copy link
Member

@u5r0 No, front and server should be completely independent (except for VScode config because VScode does not allow to scope its settings by folder, but it should not impact this PR)

@charlesBochet charlesBochet mentioned this pull request Dec 1, 2023
Closed
@u5r0 u5r0 force-pushed the vite5 branch 5 times, most recently from 910e1d3 to 965fc3c Compare December 4, 2023 18:01
@u5r0 u5r0 force-pushed the vite5 branch 5 times, most recently from b1c896c to b61de83 Compare December 9, 2023 01:08
@charlesBochet
Copy link
Member

@u5r0 I will tentatively merge your PR (switching to vite) and move to a mono-repo architecture this week-end!

@u5r0
merge squashed
d3905e8 
- A couple of CJS modules into ESM (config mostly)
- Vite complains about node.js modules: fixed `useIsMatchingLocation.ts`
	> or use rollupOptions in vite.config.ts
	> ref: https://github.com/saleor/saleor-dashboard/blob/f0e4f59d97e2a8c3e22bd2af7b7ce68a361fc9a4/vite.config.js#L6
- Adjust Storybook to work with Vite: use @storybook/test
- Use SWC for jest tranformations
- Remove unused deps:
	- ts-jest: replaced with @swc/jest, typecheck by `tsc`
	- babel plugins
	- @svgr/plugin-jsx: not used
	- @testing-library/user-event: handled by @storybook/test
	- @typescript-eslint/utils: was not plugged in
	- tsup, esbuild-plugin-svgr: will look into that later
- Install Vite required deps, and remove craco/webpack deps
- Adjust SVG to work with Vite as components
- Fixed `Step.tsx`: I dont know if one should be swaped for the other,
  but there should be no slash
- Initial formating and linting:
	- removed empty object params
	- sorting imports, etc..
@u5r0
prettier: fix pattern
a496060
@u5r0
coverage: sb coverage report isnt working
e716aaf
@u5r0
Add missing pieces
c24d465
@u5r0
yarn lint --fix
968db80
@u5r0
fix: scripts permissions
32079b8
@u5r0
tsc: cut errors in half
61627e6
@u5r0
fix: remove react-app-env.d.ts
581edc9
@u5r0
tsc: all fixed, except react-data-grid types issue
f3cfb6e
@u5r0
eslint: ignore env-config.js
d060dbf
@charlesBochet
Copy link
Member

This is great! It's indeed way faster and consuming less resources.
The dev experience looks good! There are small things I would like to change but we can do that after.

Thinks I have noticed:
image

I'm investigating this one

yarn build seems to be broken (I think it's because it's more rigourous on typing, I'll dive onto that too).

Then I'm in favor of merging and iterating on it during next week

@github-actions GitHub Actions
Copy link

TODOs/FIXMEs:

  • // Todo: remove usage of react-data-grid: front/src/modules/spreadsheet-import/steps/components/UploadStep/components/columns.tsx
  • // Todo: remove usage of react-data-grid: front/src/modules/spreadsheet-import/steps/components/ValidationStep/components/columns.tsx
  • // Todo: remove usage of react-data-grid: front/src/modules/spreadsheet-import/steps/components/ValidationStep/components/columns.tsx
  • // Todo: remove usage of react-data-grid: front/src/modules/spreadsheet-import/steps/components/ValidationStep/components/columns.tsx
  • // Todo: remove usage of react-data-grid: front/src/modules/spreadsheet-import/steps/components/SelectHeaderStep/components/SelectHeaderTable.tsx
  • // Todo: remove usage of react-data-grid: front/src/modules/spreadsheet-import/steps/components/ValidationStep/ValidationStep.tsx

Generated by 🚫 dangerJS against c0c8a62

@charlesBochet
Copy link
Member

Build size look similar:
image

with webpack:
image

@charlesBochet
Copy link
Member

Ok, I think it's due to our code not being correctly implemented. Icons are loaded multiple times concurrently and recoil does not like it. We can fix it by introducing an IconProvider

@charlesBochet
Copy link
Member

Let's merge it!

@charlesBochet charlesBochet merged commit a70a928 into twentyhq:main Dec 10, 2023
6 of 10 checks passed
@charlesBochet
Copy link
Member

charlesBochet commented Dec 11, 2023 •
edited

@u5r0 Twenty-front is now using vite! Congratulations and thank you for the work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
PR: awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@u5r0 @charlesBochet