Skip to content

feat(core): support macro re-export#133

Open
Zaczero wants to merge 1 commit intounplugin:mainfrom
Zaczero:zaczero/ready-ape
Open

feat(core): support macro re-export#133
Zaczero wants to merge 1 commit intounplugin:mainfrom
Zaczero:zaczero/ready-ape

Conversation

@Zaczero
Copy link

@Zaczero Zaczero commented Feb 6, 2026

Description

To support re-exporting from macro files as shown by tests/fixtures/re-export.js

Linked Issues

Additional context

@bolt-new-by-stackblitz
Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

@Zaczero
Copy link
Author

Zaczero commented Feb 7, 2026

@sxzz from https://github.com/antfu/contribute:

Please be aware that vibe-coding contributions are 🚫 STRICTLY PROHIBITED

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.

@sxzz
Copy link
Member

sxzz commented Feb 7, 2026

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.

Copy link
Member

@sxzz sxzz left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +388 to +381
async function resolveMacroModule(
source: string,
runner: ViteNodeRunner,
Copy link
Member

Choose a reason for hiding this comment

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

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.

@Zaczero Zaczero requested a review from sxzz February 7, 2026 13:02
@Zaczero
Copy link
Author

Zaczero commented Feb 7, 2026

If you'd like to avoid this kind of situation in the future, I'd appreciate more context in PRs

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.

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.

2 participants