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: implement @lingui/vite-plugin #1306

Merged
merged 3 commits into from
Jan 30, 2023

Conversation

timofei-iatsenko
Copy link
Collaborator

Implementation of Vite Plugin based on Snowpack Plugin

  1. Although webpack and probably snowpack loaders may work with extensions different from .po this plugin works only with .po. This is because we need a certain way to tell Vite to use proper loader for extension. Therefore files with json extension would be processed by Vite itself, and it's quite uncertain how to tell vite to use this loader for some subset of json files (we probably can play with query parameters in request, but I would prefer avoid it because .po is recommended way and everybody should use it.
  2. Naming, vite documentation https://vitejs.dev/guide/api-plugin.html#conventions clearly states that you should use vite-plugin- prefix, but in this project other plugins have own internal naming convention. I followed this convention instead of vite's. I left decision which one is right up to maintaners.

@vercel
Copy link

vercel bot commented Nov 30, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 30, 2023 at 4:28PM (UTC)

@timofei-iatsenko timofei-iatsenko changed the title implement @lingui/vite-plugin feat: implement @lingui/vite-plugin Nov 30, 2022
@timofei-iatsenko
Copy link
Collaborator Author

Tests failing because vite is in dev deps which requires min node v16. Do we really need to support all this ancient node versions?

@timofei-iatsenko
Copy link
Collaborator Author

by the way configuration of monorepo is also not well ergonomic, I would like to add "real" test for this plugin involving execution of vite itself, but it's not possible because ESM/CJS inconsistency and lack of ability to change jest config exactly for this subproject.

@andrii-bodnar
Copy link
Contributor

@thekip will it work on Node 14?

Currently, I am upgrading the CI/CD (#1325) and Node 14 - now is the min version for tests (since this version LTS period going to end on 2023-04-30)

@timofei-iatsenko
Copy link
Collaborator Author

I'm not sure actually. Vite probably requires node v16 so this will not work.

I think the whole pipeline should be changed to be independent for each package.

@andrii-bodnar
Copy link
Contributor

It's quite a big change for the entire build and release process

@timofei-iatsenko
Copy link
Collaborator Author

It's quite a big change for the entire build and release process

Yes I know.

Looks like bumping node to v14 would be enough for now

error [email protected]: The engine "node" is incompatible with this module. Expected version "^14.18.0 || >=16.0.0". Got "12.22.12"

@andrii-bodnar
Copy link
Contributor

@thekip could you please rebase the branch?

@andrii-bodnar andrii-bodnar added the ↻ awaiting rebase Rebase is needed due to conflicts label Jan 11, 2023
@andrii-bodnar andrii-bodnar linked an issue Jan 11, 2023 that may be closed by this pull request
@timofei-iatsenko
Copy link
Collaborator Author

@andrii-bodnar rebased

Copy link
Contributor

@Martin005 Martin005 left a comment

Choose a reason for hiding this comment

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

Please fix the failing validate workflow job.
You should also remove the empty CHANGELOG.md file.

packages/vite-plugin/README.md Outdated Show resolved Hide resolved
packages/vite-plugin/test/__snapshots__/index.ts.snap Outdated Show resolved Hide resolved
packages/vite-plugin/src/index.ts Show resolved Hide resolved
@timofei-iatsenko
Copy link
Collaborator Author

It's also should be registered in scripts/build/bundles. otherwise it would not be picked up by build system

@Martin005
Copy link
Contributor

It's also should be registered in scripts/build/bundles. otherwise it would not be picked up by build system

Forgot to mention that, thanks!

@andrii-bodnar
Copy link
Contributor

@thekip is it possible to finalize and release this PR?

Also, I would suggest adding the corresponding docs article within this PR so users would be able to easily find this info

@github-actions
Copy link

github-actions bot commented Jan 27, 2023

size-limit report 📦

Path Size
./packages/core/build/esm/index.js 2.89 KB (0%)
./packages/detect-locale/build/esm/index.js 804 B (0%)
./packages/react/build/esm/index.js 2.61 KB (0%)
./packages/remote-loader/build/esm/index.js 7.36 KB (0%)

@timofei-iatsenko

This comment was marked as resolved.

@timofei-iatsenko
Copy link
Collaborator Author

The scope of this PR was expanded. I tried to integrate it into the current build process and found that it not generates typings for BundleType.NODE packages. I made an ad-hoc solution with "BundleType.CUSTOM" which just call an arbitrary command.

And create an isolated command for building the package with tsc

@timofei-iatsenko
Copy link
Collaborator Author

timofei-iatsenko commented Jan 27, 2023

I also noticed what could be improved in this plugin. This plugin can compile only *.po catalogs.

The issue wouldn'be fixed by just changing a regexp to accepting *.json extension because this will break applications in general. We could not distinguish catalogs from other json files in the project using only extension.

So better solution would be take a catalog's regexp from lingui config and treat all files matching this regexp as catalogs no matter of extension.

Probably snowpack plugin also has this issue, it treat all json files in bundle as catalogs and tries to compile all of them.

I think for the first iteration we could merge it as-is (with po support only) and improve later on. Let me know what do you think.

packages/vite-plugin/README.md Outdated Show resolved Hide resolved
packages/vite-plugin/package.json Outdated Show resolved Hide resolved
website/docs/ref/vite-plugin.md Outdated Show resolved Hide resolved
website/docs/ref/vite-plugin.md Outdated Show resolved Hide resolved
website/docs/ref/vite-plugin.md Show resolved Hide resolved
@andrii-bodnar
Copy link
Contributor

andrii-bodnar commented Jan 27, 2023

@thekip thank you. I think we can start from the po support only and then will see according to users' requests.

Regarding the comment about the CHANGELOG.md file - probably we should leave it empty as is and it will be updated during the release automatically.

@timofei-iatsenko
Copy link
Collaborator Author

done

@andrii-bodnar
Copy link
Contributor

@thekip one more thing regarding the packages/cli/api.ts rename - is it something intentional?

@timofei-iatsenko
Copy link
Collaborator Author

Yes. With tsc it's not possible to use path mapping. If you noticed, the tsconfig in vite-plugin is not extended from the root one. Because in this case tsc generates structure choosing root as longest path.

Example:
./build/projects/vite-plugin/src/index.js

So I was forced not to use tsconfig's path mapping and use yarn workspace linking instead. Renaming api.js to .ts was needed to make typings works and satisfy compiler

website/docs/ref/vite-plugin.md Outdated Show resolved Hide resolved
website/docs/ref/vite-plugin.md Outdated Show resolved Hide resolved
scripts/build/bundles.ts Outdated Show resolved Hide resolved
scripts/build/custom.ts Outdated Show resolved Hide resolved
@timofei-iatsenko
Copy link
Collaborator Author

done

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Base: 82.60% // Head: 82.68% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (7771281) compared to base (ad9b735).
Patch coverage: 89.83% of modified lines in pull request are covered.

❗ Current head 7771281 differs from pull request most recent head 66bf39d. Consider uploading reports for the commit 66bf39d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1306      +/-   ##
==========================================
+ Coverage   82.60%   82.68%   +0.07%     
==========================================
  Files          56       66      +10     
  Lines        1799     1761      -38     
  Branches      506      483      -23     
==========================================
- Hits         1486     1456      -30     
+ Misses        183      177       -6     
+ Partials      130      128       -2     
Impacted Files Coverage Δ
packages/cli/src/api/formats/index.ts 60.00% <ø> (ø)
packages/conf/src/index.ts 86.07% <ø> (ø)
packages/core/src/formats.ts 84.37% <ø> (ø)
packages/core/src/index.ts 100.00% <ø> (ø)
packages/macro/src/utils.ts 100.00% <ø> (ø)
packages/react/src/Trans.tsx 80.64% <ø> (ø)
packages/remote-loader/src/index.ts 66.66% <ø> (ø)
packages/remote-loader/src/browserCompiler.ts 40.00% <40.00%> (-24.87%) ⬇️
packages/cli/src/api/detect.ts 57.89% <66.66%> (-6.82%) ⬇️
packages/core/src/context.ts 80.00% <77.77%> (+0.37%) ⬆️
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@andrii-bodnar
Copy link
Contributor

Thank you @thekip, @Martin005! I'm going to release a new version later this week

@andrii-bodnar andrii-bodnar merged commit db5d3c3 into lingui:main Jan 30, 2023
@timofei-iatsenko timofei-iatsenko deleted the feature/vite-plugin branch January 31, 2023 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Official vite support?
3 participants