fix: improve detection of comment directives#226
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #226 +/- ##
==========================================
+ Coverage 94.74% 95.03% +0.28%
==========================================
Files 9 10 +1
Lines 2265 2495 +230
==========================================
+ Hits 2146 2371 +225
- Misses 119 124 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
BTW, i think this could be solved by enabling /**
* Run Wasm plugins before stripping TypeScript or decorators.
*
* See https://github.com/swc-project/swc/issues/9132 for more details.
*/
runPluginFirst?: boolean;This option is poorely documented, i've found it in the PR |
|
Here is few issues with this implementation:
I don't see any other solution rather to do what it's already done here - parse comments manually. Seems SWC doesn't give us any other way. But some changes should be done here:
|
|
@timofei-iatsenko Thanks for reviewing! I pushed a commit that should address everything. Ended up with a minimal JS parser that only looks for comments, to avoid false positives like |
|
I added a benchmark suite for lingui cli/transforms i'm going to validate this solution using this benchmark first. |
4db6e2f to
885b7d9
Compare
|
I checked implementation with benchmark and it doesn't add any significant peformance overhead, so we good to go. I refactored/cleaned up implementation:
|
mogelbrod
left a comment
There was a problem hiding this comment.
Looks great, thanks @timofei-iatsenko!
|
I added few more changes, this ready to be merged. |
Fix
lingui-setdirectives disappearing in the SWC plugin when they appear before TypeScript-only declarations, such asexport type.lingui-setcontext was sometimes lost during SWC transform. This resulted in message IDs were generated without the directive context, so the transformed app code used different IDs from the extracted catalogs.In practice, a file like this could produce the wrong ID:
Instead of using the
navigationcontext and generating the context-aware ID, the transform fell back to the unscoped ID.Root cause
The plugin’s existing directive lookup depended on SWC comment attachment by node position. That works when the directive comment is attached to a visited runtime node, but it breaks for comments that sit before TypeScript-only declarations or before the first import.
There were two related gaps:
program.span(), which starts at the first AST token, not necessarily at the beginning of the file. That means top-of-filelingui-setcomments could be excluded from the fallback source text entirely.So the plugin could miss a valid directive even though the source file clearly contained it.
Solution
This change adds a source-text-based directive fallback and wires it into directive lookup:
lingui-set/lingui-resetdirectives directly from source lines.MacroCtx.program.span().lo, so top-of-file directives are included.Why this approach
SWC doesn't appear to expose a global “all comments” API in plugin mode.
This solution was selected because it fixes the actual failure mode without depending on such an API.
Other options were weaker:
Using source text as a fallback is more robust because directives are fundamentally lexical comments, and this approach preserves the existing fast positional path while covering the cases SWC comment attachment misses.
Validation
Added/updated E2E coverage for a TypeScript case where
lingui-setappears beforeexport type, and verified that the transform now preserves the expected context-aware ID instead of the unscoped one.