Mark @fedify/fixture as external in test build configs#635
Conversation
During `pnpm pack --recursive`, tsdown tried to bundle @fedify/fixture in the test build configs of several packages. Since @fedify/fixture is a private workspace package, its dist/ may not exist when parallel prepack builds run, causing UNLOADABLE_DEPENDENCY errors. Add "@fedify/fixture" to the external array in the test build config of: - @fedify/webfinger - @fedify/vocab - @fedify/fedify - @fedify/vocab-runtime This aligns them with @fedify/testing, which already correctly externalizes @fedify/fixture. Co-authored-by: Amp (https://ampcode.com) Amp-Thread-ID: https://ampcode.com/threads/T-019d00c0-4bd0-70fe-b81f-82e3738959e9 Co-authored-by: Amp <amp@ampcode.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical build failure encountered during the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
@codex review |
dahlia
added
type/bug
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a build issue where @fedify/fixture was being bundled by tsdown in test configurations, causing UNLOADABLE_DEPENDENCY errors. By marking @fedify/fixture as an external dependency in the tsdown.config.ts files for fedify, vocab-runtime, vocab, and webfinger, you ensure that the bundler does not try to include it in the build output.
My review found one related issue: the package.json for @fedify/vocab-runtime is missing @fedify/fixture in its devDependencies. While your change prevents the bundling error, the missing dependency declaration could cause runtime errors in Node.js or Bun environments. I've left a comment with details on how to resolve this to ensure consistency with the project's contribution guidelines.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f3576f3c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
…e bundled
The cfworkers test harness imports dist/testing/mod.js, which re-exports
testDefinitions from @fedify/fixture. Marking @fedify/fixture as
external in that build config would break test:cfworkers since the
private package would not be available at runtime in Wrangler.
Split the second defineConfig into two:
- One for src/testing/mod.ts (without @fedify/fixture external),
so cfworkers can bundle it
- One for src/**/*.test.ts (with @fedify/fixture external),
so pnpm pack --recursive does not try to resolve it
fedify-dev#635 (comment)
Co-authored-by: Amp (https://ampcode.com)
Amp-Thread-ID: https://ampcode.com/threads/T-019d00c0-4bd0-70fe-b81f-82e3738959e9
Co-authored-by: Amp <amp@ampcode.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly identifies and fixes a build issue where @fedify/fixture was being bundled during test builds, causing errors with pnpm pack. The changes across webfinger, vocab, and vocab-runtime correctly mark @fedify/fixture as an external dependency.
In packages/fedify/tsdown.config.ts, you've split the configuration, which is a good approach. However, it appears that @fedify/fixture was missed in the external array for the src/testing/mod.ts build configuration. Since this module re-exports from @fedify/fixture, it should also be treated as external to prevent bundling issues. I've added a suggestion to address this.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 484b5b45b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The previous approach of splitting src/testing/mod.ts into a separate
defineConfig caused the cfworkers build to fail because tsdown flattened
the single-entry output to dist/mod.js instead of dist/testing/mod.js,
breaking the import in src/cfworkers/server.ts.
Instead, keep src/testing/mod.ts and *.test.ts in a single defineConfig
(preserving directory structure in output), mark @fedify/fixture as
external, and use noExternal with a function to selectively bundle it
back in only when imported from src/testing/ files. This way:
- The cfworkers build gets @fedify/fixture inlined in testing/mod.js
- Test files keep @fedify/fixture external, avoiding the
UNLOADABLE_DEPENDENCY error during pnpm pack --recursive
https://github.com/fedify-dev/fedify/actions/runs/23243929108/job/67567201018
Co-authored-by: Amp (https://ampcode.com)
Amp-Thread-ID: https://ampcode.com/threads/T-019d00c0-4bd0-70fe-b81f-82e3738959e9
Co-authored-by: Amp <amp@ampcode.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a build failure with pnpm pack --recursive by marking the private workspace package @fedify/fixture as external in the test build configurations for several packages. The changes are logical and consistent across the modified files. However, I've identified a potential edge case in the noExternal logic for the @fedify/fedify package where the path matching could be too broad. My review includes a suggestion to make this logic more robust.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb6924c1f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Use "/src/testing/" instead of "/testing/" in the noExternal importer path check to avoid false positives when the project itself is located under a directory path containing "/testing/". fedify-dev#635 (comment) Co-authored-by: Amp (https://ampcode.com) Amp-Thread-ID: https://ampcode.com/threads/T-019d00c0-4bd0-70fe-b81f-82e3738959e9 Co-authored-by: Amp <amp@ampcode.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a build issue by marking @fedify/fixture as an external dependency in the test build configurations for several packages. The changes in the tsdown.config.ts files are appropriate and should resolve the pnpm pack failure. I have one minor suggestion for the documentation to maintain consistency in heading capitalization.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd336d2a5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Closes #634
Summary
pnpm pack --recursivefails because several packages' tsdown test build configurations try to bundle@fedify/fixtureinstead of treating it as an external dependency. Since@fedify/fixtureis a private workspace package, its dist/ may not exist when parallel prepack builds run, causingUNLOADABLE_DEPENDENCYerrors.Changes
Added
"@fedify/fixture"to theexternalarray in the test build config of:This aligns them with
@fedify/testing, which already correctly externalizes@fedify/fixture.