Conversation
|
|
There was a problem hiding this comment.
Pull request overview
Adds support in the core macro transformer for re-exporting values directly from macro modules (via export ... from '...' with { type: 'macro' }), with new fixtures validating named exports, export-all, and namespace export behavior.
Changes:
- Detect and transform macro re-export declarations (
ExportNamedDeclaration/ExportAllDeclaration) alongside existing macro import/call handling. - Add fixtures for successful macro re-exports and for missing re-exported macro symbols.
- Update fixture snapshots to reflect the new transformation outputs and error cases.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures/re-export.js | New fixture covering named re-exports, default re-export, export-all, and namespace export from macro modules. |
| tests/fixtures/macros/export-all.ts | New macro module used by the re-export fixture to validate default + named exports. |
| tests/fixtures/error-re-export-not-existed.js | New fixture ensuring a clear error is thrown when re-exporting a non-existent macro symbol. |
| tests/snapshots/fixtures.test.ts.snap | Snapshot updates for new fixtures and new expected output. |
| src/core/index.ts | Core implementation: detect macro re-export declarations and execute them at transform time by materializing runtime exports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1e3abae to
4058fd1
Compare
|
@sxzz from https://github.com/antfu/contribute:
Please respect others' time and avoid vibe-coding your PR reviews. I have never seen a single actually good GitHub Copilot PR review. I always waste my time reading suggestions that don't even make sense. Please, at least use a better tool, and maybe review the suggestions yourself. Let me recommend CodeRabbit or Codex Code Review as alternatives. |
|
I understand the frustration with low-quality AI-generated reviews, and I personally have been bothered by vibe-coded PRs recently as well — so trust me, I wouldn't want to do the same thing to contributors. That said, using GitHub Copilot Review here wasn't meant to waste your time. I used it primarily to help me get a quick overview of the changes in this PR, since the description only had a single line. The overview summary it generated was actually helpful for that purpose. If you'd like to avoid this kind of situation in the future, I'd appreciate more context in PRs — ideally opening an issue first to discuss the feature, or at least providing a more detailed description of what the PR does and why. A one-line description makes it harder for maintainers to understand the intent quickly, and that's why I relied on tooling to help. I appreciate your contribution and I'll review the code itself. But suggesting that I'm wasting your time or disrespecting your work by using a review tool — while providing minimal context yourself — isn't quite fair. |
sxzz
left a comment
There was a problem hiding this comment.
Thanks for the PR. The feature makes sense and the test coverage is solid (named re-exports, default, export *, export * as ns, and the error case).
Main concern is the duplicated module resolution logic — see inline comment. Other than that, this looks reasonable.
| async function resolveMacroModule( | ||
| source: string, | ||
| runner: ViteNodeRunner, |
There was a problem hiding this comment.
This duplicates the module resolution logic already in executeMacro (resolveUrl → isBuiltin check → import/executeFile). Consider refactoring executeMacro to call resolveMacroModule instead of inlining the same pattern, so there's a single place to maintain.
Also note the dep tracking is inconsistent: here deps.get(id)!.add(resolved) happens immediately, but in executeMacro it's done at the end after execution. Should be consistent.
4058fd1 to
40bcc1d
Compare
Sure, will do that. I can see my PR description being poor. From my perspective, it was GH Copilot pointing out non-issues without any further message for 3h (as if you wanted me to check their review). I really dislike GitHub Copilot reviews because, from my experience, they are the worst. So perhaps we are both right. |
Description
To support re-exporting from macro files as shown by tests/fixtures/re-export.js
Linked Issues
Additional context