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(): Experimental Deps extractor #1469

Merged
merged 5 commits into from
Mar 15, 2023

Conversation

timofei-iatsenko
Copy link
Collaborator

@timofei-iatsenko timofei-iatsenko commented Feb 28, 2023

Implements: #1458

Process overview:

  • Take glob of entrypoints src/pages/**/*.ts resolve them into paths
  • Send paths to the esbuild as entrypoints
  • ESBuild bundle each entrypoint traversing all included modules.
    • All external dependencies excluded from bundling see buildExternalizeFilter test. Externalization can be configured with includeDeps: string[] and excludeDeps: string[]
    • All resource files such as css, svg, jpg, etc excluded from bundling
    • tsconfig path resolution algorithm is respected (built-in in esbuild)
  • Run extraction process (with babel) on resulting files (bundles) and get catalog per each bundle.

Notes

  • Extractor cli command has "experimental" in it's name. I want to release it as-is and start collecting feedback. During this "experimanetal" phase, implementation and interface may change without bumping a major version. (such banner added to the command output)
  • ESBuild bundle so fast that that time can be ignored
  • ESBuild is used just for dependency analyzation, it doesn't matter that target project may not work with esbuild because of some specific transforms. As long source is valid ES code it should work.
  • Extraction from resulting bundles is noticeable slowly because bundles may contain common files and extractor do the same work.
  • We can implement multithreading for extracting from bundles, but currently it's blocked because of @lingui/conf (i will explain later)
  • The new extraction scheme affects compiling as well. Because compiler should know where to look catalogs.
  • i added a comprehensive e2e to the whole extraction/compilation process. Many files in the PR is test fixtures and snapshots.
  • Extractor already support extracting to template and merging with existing
  • Extending extractor with Vue or Svelte support is possible, but might be tricky.

Regarding multithreading:

Communication between threads is done by messages. Where message should be a serializable structure.
Extractor workers depedns on the @lingui/config, but lingui config itself might not be serializable to json. Custom extractor might be defined as an inline function in the config.

So passing config from parent process to childs is not possible because of that.

Other way would be read config in each worker separately. But reading config has significant overhead especially when config written in typescript. Recursive search and parsing / compiling, would be called in each worker, so overhead could be bigger than just doing it all in one thread.

So there are 2 possible solutions:

  • Forbid functions and other non-serializable objects in the config. This is Jest way. Instead of function user should pass a path to the module (f.ex using require.resolve)
  • Prebuild config. This is Vite way. They are bundling config with esbuild and store it in the temp folder. Then each subsequent invocation use already compiled pure js config.

First option is simpler, but has bad DX, second option is better for DX, but harder for implementation.

@vercel
Copy link

vercel bot commented Feb 28, 2023

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 Mar 10, 2023 at 10:32AM (UTC)

@github-actions
Copy link

github-actions bot commented Feb 28, 2023

size-limit report 📦

Path Size
./packages/core/build/esm/index.js 1.56 KB (0%)
./packages/detect-locale/build/esm/index.js 812 B (0%)
./packages/react/build/esm/index.js 1.79 KB (0%)
./packages/remote-loader/build/esm/index.js 7.25 KB (0%)

@timofei-iatsenko

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Patch coverage: 81.37% and project coverage change: +0.37 🎉

Comparison is base (ec95916) 73.93% compared to head (4753824) 74.30%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1469      +/-   ##
==========================================
+ Coverage   73.93%   74.30%   +0.37%     
==========================================
  Files          67       76       +9     
  Lines        1872     1969      +97     
  Branches      493      514      +21     
==========================================
+ Hits         1384     1463      +79     
- Misses        383      392       +9     
- Partials      105      114       +9     
Impacted Files Coverage Δ
packages/cli/src/api/catalog/mergeCatalog.ts 93.33% <ø> (ø)
packages/cli/src/lingui.ts 0.00% <ø> (ø)
packages/conf/src/makeConfig.ts 100.00% <ø> (ø)
...ges/cli/src/extract-experimental/getEntryPoints.ts 50.00% <50.00%> (ø)
packages/cli/src/lingui-extract-experimental.ts 60.60% <60.60%> (ø)
...src/extract-experimental/buildExternalizeFilter.ts 83.33% <83.33%> (ø)
...rc/extract-experimental/getExperimentalCatalogs.ts 87.50% <87.50%> (ø)
...kages/cli/src/extract-experimental/bundleSource.ts 93.33% <93.33%> (ø)
...ages/cli/src/extract-experimental/writeCatalogs.ts 93.75% <93.75%> (ø)
packages/cli/src/api/catalog.ts 90.00% <100.00%> (-0.22%) ⬇️
... and 5 more

... and 1 file with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@andrii-bodnar
Copy link
Contributor

@thekip I already briefly checked this PR and going to explore it in more detail later this week

@andrii-bodnar andrii-bodnar removed the request for review from Martin005 March 9, 2023 08:09
@andrii-bodnar
Copy link
Contributor

@thekip thank you, looks good to me! 🚀

I just left two minor comments. Also, please add a section to the migration guide about this new extractor.

@andrii-bodnar andrii-bodnar added the ↻ awaiting rebase Rebase is needed due to conflicts label Mar 10, 2023
@timofei-iatsenko
Copy link
Collaborator Author

Also, please add a section to the migration guide about this new extractor.

It's not replacing the current extractor, so no migration is needed. It's just an alternative. I want to test this on different projects and check what errors might appear, what should be changed in the design of the api and configurations and so on.

Another question how configuration would look like when it becomes "stable" instead of "experimental". We need to merge configurations somehow.

I actually have no idea where to write this. I think we need to write a blog post with new capabilities, such as typescript doing for new versions.

@andrii-bodnar
Copy link
Contributor

@thekip ok, we can keep it not documented for the test period. I consider the migration article as a place for letting users know about new features also.

On the official v4 release probably we will create a blog post with everything included.

Another question how configuration would look like when it becomes "stable" instead of "experimental"

can we keep both the approaches (current and this experimental) as alternative solutions? The current will be default and if needed, users may use this "advanced" extractor

@timofei-iatsenko
Copy link
Collaborator Author

It's still requiring proper naming. Because current "experimental" is not a good for the future. So settings for both two extractors should somehow play well with each other.

Actually, i see something like in tsconfig right now. You have two options to choose:

  • include or exclude - this way files discovered by glob patterns
  • or files, files discovered and transpiled by deps graph.

I think something similar could be implemented in lingui as well. Both versions of extractors would live side by side, and users event would not know that there two diffrent implementations it should be just a matter of different configuration.

But i'm going to consider this for next major release (v5 for example)

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

@andrii-bodnar merge?

@andrii-bodnar
Copy link
Contributor

andrii-bodnar commented Mar 13, 2023

@thekip sure, but I'm still concerned about adding instructions on configuring this extractor. Users won't know about this feature. Maybe, the Monorepo page is a good place for that?

@timofei-iatsenko
Copy link
Collaborator Author

let's merge this, i will write docs in follow ups. I'm going to add a dedicated nextjs doc, and dedicated page for that extractor. But first i want to finish all ongoing PR, and there already would be conflicts between this and custom formatters.

@andrii-bodnar andrii-bodnar merged commit fbdeffb into lingui:next Mar 15, 2023
@timofei-iatsenko timofei-iatsenko deleted the experimental-extractor branch March 16, 2023 12:48
@andrii-bodnar andrii-bodnar mentioned this pull request Mar 21, 2023
8 tasks
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.

RFC: deps tree Extractor. Support MPA apps (NextJS) and monorepos. Use lingui.js with multiple packages
2 participants