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

feat: i18n - dynamic import on build time #188

Merged
merged 7 commits into from
May 1, 2024
Merged

Conversation

aruznieto
Copy link
Contributor

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

@aruznieto aruznieto requested a review from dec0dOS as a code owner October 20, 2023 09:12
@@ -0,0 +1,10 @@
[
{
Copy link
Owner

Choose a reason for hiding this comment

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

How about just moving the code field to the locale files? We could get it from them as well, no need it having additional file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need to generate the file, because before it reads on i18n.js, and Settings.jsx file

Copy link
Owner

Choose a reason for hiding this comment

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

We need to refactor it in some way. Explicit build step introduce so much additional caveats and complexity in the long run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If does not want to read the content of the folders at "real-time" (not at building) you need to generate the JSON file. Merge all translations in 1 file I think is not a good idea

Copy link
Owner

Choose a reason for hiding this comment

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

I need time to think about it. Anyway, build step should always be at the bundler level
E.g.: https://github.com/egoist/vite-plugin-compile-time

Copy link
Owner

@dec0dOS dec0dOS Oct 21, 2023

Choose a reason for hiding this comment

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

In simple terms, I have some concerns regarding generateLocalesList.js and frontend/src/utils/localesList.json.

I suggest making the following adjustments:

The first file, generateLocalesList.js, should be introduced as a build step plugin.
The second file, localesList.json, should be generated only after build step and go to build directory. It's important to note that this file should never be committed to the repository since it's considered generated content. There could be some caveats in making it work as expected during the dev stage in Vite. Need to check it how to solve that correctly.

Copy link
Contributor Author

@aruznieto aruznieto Oct 21, 2023

Choose a reason for hiding this comment

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

But If you move it to build folder, how can you read it on react files? With fetch?

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Some vite plugin should handle this. Or another option (more simple one) just to place localesList.json in public directory (I think that it's more suitable location) and gitignore just that file
Anyway, current solution is far better than before, as explicit build step with calling custom script was really confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this option could be a solution https://vitejs.dev/config/build-options.html#build-dynamicimportvarsoptions https://github.com/rollup/plugins/tree/master/packages/dynamic-import-vars

But how can you import a file that you generate at build time?

Copy link
Contributor Author

@aruznieto aruznieto left a comment

Choose a reason for hiding this comment

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

👍🏻

@aruznieto
Copy link
Contributor Author

Hi, whats happened with that?

@dec0dOS dec0dOS merged commit 0fb9264 into dec0dOS:main May 1, 2024
1 check passed
@dec0dOS
Copy link
Owner

dec0dOS commented May 1, 2024

LGTM, ready to merge. I tested extensively some big changes on my local setup before releasing the production image. I'm really busy with some projects now, so please let me know if that works for your setup as expected

@aruznieto
Copy link
Contributor Author

aruznieto commented May 1, 2024

LGTM, ready to merge. I tested extensively some big changes on my local setup before releasing the production image. I'm really busy with some projects now, so please let me know if that works for your setup as expected

Do you know why appears this error on yarn install?

error Workspaces can only be enabled in private projects.

@dec0dOS
Copy link
Owner

dec0dOS commented May 1, 2024

Seems like some errors in yarn.lock. Get the fixed version from main branch.

@aruznieto
Copy link
Contributor Author

Seems like some errors in yarn.lock. Get the fixed version from main branch.

I am cloning the repo

@dec0dOS
Copy link
Owner

dec0dOS commented May 1, 2024

Make sure that you use the latest yarn version (corepack should help)

❯ yarn -v
4.1.1
❯ node -v
v20.10.0
❯ cd /tmp/zero-ui
❯ git pull
Already up to date.
❯ ls
backend       docker-compose.yml  LICENSE       README.md
CHANGELOG.md  docs                node_modules  tsconfig.json
docker        frontend            package.json  yarn.lock
❯ rm -rf node_modules
❯ yarn install
➤ YN0000: · Yarn 4.1.1
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed
➤ YN0000: ┌ Fetch step
➤ YN0000: └ Completed in 0s 269ms
➤ YN0000: ┌ Link step
➤ YN0007: │ zero-ui@workspace:. must be built because it never has been before or the last one failed
➤ YN0007: │ esbuild@npm:0.18.20 must be built because it never has been before or the last one failed
➤ YN0000: └ Completed in 6s 563ms
➤ YN0000: · Done in 7s 102ms
❯ yarn install
➤ YN0000: · Yarn 4.1.1
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed
➤ YN0000: ┌ Fetch step
➤ YN0000: └ Completed in 0s 275ms
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed
➤ YN0000: · Done in 0s 612ms
❯ yarn build
vite v4.5.3 building for production...
Locales list saved to /private/tmp/zero-ui/frontend/src/generated/localesList.json
✓ 1234 modules transformed.
build/index.html                                              0.67 kB │ gzip:   0.37 kB
build/assets/roboto-vietnamese-400-normal-77b24796.woff2      5.56 kB
build/assets/roboto-greek-400-normal-daf51ab5.woff2           7.11 kB
build/assets/roboto-cyrillic-400-normal-495d38d4.woff2        9.63 kB
build/assets/roboto-latin-ext-400-normal-3c23eb02.woff2      11.87 kB
build/assets/roboto-cyrillic-ext-400-normal-b7ef2cd1.woff2   15.34 kB
build/assets/roboto-latin-400-normal-f6734f81.woff2          15.74 kB
build/assets/roboto-all-400-normal-e41533d5.woff             65.46 kB
build/assets/index-81542e93.css                              12.40 kB │ gzip:   4.32 kB
build/assets/index-b9b777a6.js                              901.67 kB │ gzip: 294.38 kB
✓ built in 3.21s

@aruznieto
Copy link
Contributor Author

Make sure that you use the latest yarn version (corepack should help)

❯ yarn -v
4.1.1
❯ node -v
v20.10.0
❯ cd /tmp/zero-ui
❯ git pull
Already up to date.
❯ ls
backend       docker-compose.yml  LICENSE       README.md
CHANGELOG.md  docs                node_modules  tsconfig.json
docker        frontend            package.json  yarn.lock
❯ rm -rf node_modules
❯ yarn install
➤ YN0000: · Yarn 4.1.1
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed
➤ YN0000: ┌ Fetch step
➤ YN0000: └ Completed in 0s 269ms
➤ YN0000: ┌ Link step
➤ YN0007: │ zero-ui@workspace:. must be built because it never has been before or the last one failed
➤ YN0007: │ esbuild@npm:0.18.20 must be built because it never has been before or the last one failed
➤ YN0000: └ Completed in 6s 563ms
➤ YN0000: · Done in 7s 102ms
❯ yarn install
➤ YN0000: · Yarn 4.1.1
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed
➤ YN0000: ┌ Fetch step
➤ YN0000: └ Completed in 0s 275ms
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed
➤ YN0000: · Done in 0s 612ms
❯ yarn build
vite v4.5.3 building for production...
Locales list saved to /private/tmp/zero-ui/frontend/src/generated/localesList.json
✓ 1234 modules transformed.
build/index.html                                              0.67 kB │ gzip:   0.37 kB
build/assets/roboto-vietnamese-400-normal-77b24796.woff2      5.56 kB
build/assets/roboto-greek-400-normal-daf51ab5.woff2           7.11 kB
build/assets/roboto-cyrillic-400-normal-495d38d4.woff2        9.63 kB
build/assets/roboto-latin-ext-400-normal-3c23eb02.woff2      11.87 kB
build/assets/roboto-cyrillic-ext-400-normal-b7ef2cd1.woff2   15.34 kB
build/assets/roboto-latin-400-normal-f6734f81.woff2          15.74 kB
build/assets/roboto-all-400-normal-e41533d5.woff             65.46 kB
build/assets/index-81542e93.css                              12.40 kB │ gzip:   4.32 kB
build/assets/index-b9b777a6.js                              901.67 kB │ gzip: 294.38 kB
✓ built in 3.21s

I just tested it, the i18n works perfect

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.

2 participants