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

Exclude type-checking for i18n & Add workflow for full check #718

Merged
merged 7 commits into from
Jan 24, 2025

Conversation

zyf722
Copy link
Contributor

@zyf722 zyf722 commented Jan 17, 2025

What type of PR is this? (check all applicable)

  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • ♻️ Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert
  • 🌐 Internationalization / Translation

Description

This PR solves the build performance bottleneck raised in #707, which was caused by type-checking the entire codebase (including i18n locale objects) during building.

A new tsconfig.noI18n.json file that extends the base tsconfig is added to exclude the src/livecodes/i18n/locales directory for building types. A new workflow to perform full type-checking (triggered by PRs and pushes) is also included.

Related Tickets & Documents

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentations?

  • πŸ““ docs (./docs)
  • πŸ“• storybook (./storybook)
  • πŸ“œ README.md
  • πŸ™… no documentation needed

Copy link

netlify bot commented Jan 17, 2025

βœ… Deploy Preview for livecodes ready!

Name Link
πŸ”¨ Latest commit 4fa8db7
πŸ” Latest deploy log https://app.netlify.com/sites/livecodes/deploys/6792f22dc0cf550008330d5d
😎 Deploy Preview https://deploy-preview-718--livecodes.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@zyf722
Copy link
Contributor Author

zyf722 commented Jan 17, 2025

Hi @hatemhosny ,

Could you please take a review on this when you have time?

The workflow for type-checking seems to be working as expected now.

It reports that there are some i18n source changes for Code to Image feature, and translation key in other languages are inconsistent with English. We might want to fix them on Lokalise.

@hatemhosny
Copy link
Collaborator

hatemhosny commented Jan 18, 2025

Thank you @zyf722

The type check is indeed a lot faster now.

I suggest to use the opposite approach.
Let's make the exclusion in the default tsconfig file. This is used for:

  • the build:ts script.
  • the typedoc docusaurus plugin: this can make npm run docs and npm run build:docs a lot faster.
  • this is probably used by the IDE. So this might also make types in the IDE a lot faster.

We can then remove this exclusion in the other tsconfig file used by the CI workflow.

What do you think?

It reports that there are some i18n source changes for Code to Image feature, and translation key in other languages are inconsistent with English. We might want to fix them on Lokalise.

Is that related to this PR?
Can you also please check there. I think it wants to remove many keys that are actually being used.

Edit:
The type errors are now fixed after adding the missing translations and pulling from lokalise.

@hatemhosny hatemhosny enabled auto-merge January 20, 2025 01:30
@hatemhosny hatemhosny disabled auto-merge January 20, 2025 01:36
@zyf722
Copy link
Contributor Author

zyf722 commented Jan 24, 2025

Sorry for being a bit late to respond. I was kind of busy this week πŸ˜΅β€πŸ’«

Let's make the exclusion in the default tsconfig file. This is used for:

  • the build:ts script.
  • the typedoc docusaurus plugin: this can make npm run docs and npm run build:docs a lot faster.
  • this is probably used by the IDE. So this might also make types in the IDE a lot faster.

We can then remove this exclusion in the other tsconfig file used by the CI workflow.

Done.

I did not think of directly exclude them in the base tsconfig, as I subconsciously thought that these files would be used by the IDE's Intellisense. But now I realize that only English, our base language, is needed for this purpose and it will be included anyway. So it makes sense to exclude other languages - thanks for pointing this out.

Is that related to this PR? Can you also please check there. I think it wants to remove many keys that are actually being used.

Edit: The type errors are now fixed after adding the missing translations and pulling from lokalise.

I haven't had a chance to thoroughly check this PR yet. Is the weekly workflow working as we expected?

@hatemhosny
Copy link
Collaborator

Thank you @zyf722
This is very nice. Much faster!

I haven't had a chance to thoroughly check this PR yet. Is the weekly workflow working as we expected?

Yes. It is working well. Overall, the whole i18n workflow is great. Thank you.

@hatemhosny hatemhosny merged commit c3c9366 into live-codes:develop Jan 24, 2025
15 checks passed
Copy link
Contributor

livecodes-ci bot commented Jan 24, 2025

i18n Actions

Source PR has been merged into the default branch.

Maintainers can comment .i18n-update-push to trigger the i18n update workflow and push the changes to Lokalise.

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.

i18n types performance
2 participants