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

fix(webpack + vite): fix dependency watching in loader #1671

Merged
merged 3 commits into from
May 24, 2023

Conversation

timofei-iatsenko
Copy link
Collaborator

@timofei-iatsenko timofei-iatsenko commented May 24, 2023

Description

Here we go again. The deps watching still doesn't work and cause problems with cache bursting on my project.

Found that i erroneously assumed that path in the Catalog is a relative, where it actually is absolute. Based on this wrong assumption, unnecessary path concatenation was made, resulting in incorrect paths for files.

I also write a test for webpack loader checking that webpack actually watching all files and react on the changes in them.

@vercel
Copy link

vercel bot commented May 24, 2023

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

Name Status Preview Comments Updated (UTC)
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 24, 2023 0:35am

@github-actions
Copy link

github-actions bot commented May 24, 2023

size-limit report 📦

Path Size
./packages/core/dist/index.mjs 1.44 KB (0%)
./packages/detect-locale/dist/index.mjs 721 B (0%)
./packages/react/dist/index.mjs 1.59 KB (0%)
./packages/remote-loader/dist/index.mjs 7.24 KB (0%)

@timofei-iatsenko timofei-iatsenko force-pushed the fix/catalog-deps-watching branch from edb19d6 to 38b90a0 Compare May 24, 2023 10:12
@timofei-iatsenko timofei-iatsenko marked this pull request as ready for review May 24, 2023 10:29
Co-authored-by: Andrii Bodnar <andrii.bodnar@crowdin.com>
@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Patch coverage: 87.50% and project coverage change: +0.13 🎉

Comparison is base (507b33d) 75.56% compared to head (2b992cb) 75.70%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1671      +/-   ##
==========================================
+ Coverage   75.56%   75.70%   +0.13%     
==========================================
  Files          79       79              
  Lines        2018     2033      +15     
  Branches      519      520       +1     
==========================================
+ Hits         1525     1539      +14     
  Misses        379      379              
- Partials      114      115       +1     
Impacted Files Coverage Δ
packages/loader/test/compiler.ts 86.36% <86.36%> (+19.69%) ⬆️
...es/cli/src/api/catalog/getCatalogDependentFiles.ts 100.00% <100.00%> (ø)

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

@andrii-bodnar andrii-bodnar merged commit f06cdf5 into lingui:main May 24, 2023
@timofei-iatsenko timofei-iatsenko deleted the fix/catalog-deps-watching branch May 24, 2023 16:33
@taozhou-glean
Copy link
Contributor

maybe related, we start seeing following errors after the latest release:

Invalid dependencies have been reported by plugins or loaders for this module. All reported dependencies need to be absolute paths.
Invalid dependencies may lead to broken watching and caching.
As best effort we try to convert all invalid values to absolute paths and converting globs into context dependencies, but this is deprecated behavior.
Loaders: Pass absolute paths to this.addDependency (existing files), this.addMissingDependency (not existing files), and this.addContextDependency (directories).
Plugins: Pass absolute paths to fileDependencies (existing files), missingDependencies (not existing files), and contextDependencies (directories).
Globs: They are not supported. Pass absolute path to the directory as context dependencies.
The following invalid values have been reported:
 * "data/locales/web/en/messages.po"

@timofei-iatsenko
Copy link
Collaborator Author

@taozhou-glean yes, this is related, but could you create a miminal repro? Don't understand the case where relative paths happened

@taozhou-glean
Copy link
Contributor

@thekip sorry for the late reply, was busy with something else. I just checked, i think the reason is that the catalogs.path is relative in our lingui.config.ts, so it has been relative path to loader all the time. Let me see if I can fix it on our side, otherwise I think maybe we can change this behavior to check isAbsolute and continue the resolving if it's not. (we use bazel for our builds and some symlinks is involved here, so it might not be that easy to change it to absolute.

@taozhou-glean
Copy link
Contributor

ah I guess we should use <rootDir> in the path which we did not... will give that a try

@gpessa
Copy link

gpessa commented Jun 4, 2023

I still have the same issue despite the fix

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.

4 participants