Skip to content

Option to write exports to jsr.json#57

Merged
colinhacks merged 9 commits intocolinhacks:mainfrom
DallasHoff:jsr
Dec 30, 2025
Merged

Option to write exports to jsr.json#57
colinhacks merged 9 commits intocolinhacks:mainfrom
DallasHoff:jsr

Conversation

@DallasHoff
Copy link
Contributor

Implements #55

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

This PR implements JSR (JavaScript Registry) support by adding an option to automatically copy export configurations to jsr.json. When jsr: true is set in the zshy configuration, the tool will now write the source code entrypoint exports to jsr.json, making the zshy config the single source of truth for exports.

Key Changes:

  • Added jsr boolean configuration option to write exports to jsr.json
  • Refactored package.json path finding and indentation detection into reusable utility functions
  • Added comprehensive test coverage with a new test fixture in test/jsr/

Reviewed changes

Copilot reviewed 9 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/main.ts Added JSR configuration option and logic to read/write jsr.json with proper indentation preservation
src/utils.ts Extracted findConfigPath and detectConfigIndention utility functions for reuse across config file operations
test/zshy.test.ts Added test case to verify exports are copied to jsr.json when jsr option is enabled
test/jsr/* New test fixture directory with complete setup including package.json, jsr.json, source files, and build output
test/__snapshots__/zshy.test.ts.snap Added snapshot for the jsr test case showing expected build output
README.md Documented the new JSR feature and fixed quote style consistency in code examples

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@colinhacks
Copy link
Owner

cc @pullfrog

@pullfrog
Copy link
Contributor

pullfrog bot commented Dec 9, 2025

✅ Review complete for PR #57

Summary: The implementation successfully adds JSR support to zshy, allowing the tool to copy exports to jsr.json while maintaining package.json as the source of truth.

Key findings:

  • ✅ Correctly implements the requirement from issue Proposal: option to write exports to jsr.json #55
  • ✅ Properly validates that jsr.json must exist (doesn't create it)
  • ✅ Good refactoring with reusable utility functions (findConfigPath, detectConfigIndentation)
  • ✅ Includes comprehensive test coverage
  • ✅ Clear documentation in README

Feedback provided:

  • Minor organizational suggestion for config normalization
  • Suggestion to add more helpful error messaging
  • Note about unrelated style change in README
  • Question about committed dist files in test fixtures

The core implementation is solid and ready for merge with minor improvements if desired.

Pullfrog  | Triggered by Pullfrog | Using Claude CodeView workflow run𝕏

Copy link
Contributor

@pullfrog pullfrog bot left a comment

Choose a reason for hiding this comment

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

Implementation looks solid overall. The feature correctly implements the requirement from issue #55, properly validates that jsr.json exists before modifying it, and includes good test coverage.

src/main.ts Outdated

// Normalize boolean options
config.noEdit ??= false;
config.jsr ??= false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving the config.jsr normalization after the config.cjs normalization and validation block (after line 290) to keep all boolean option normalizations together in one place, rather than splitting them.

//////////////////////////////////

if (config.jsr) {
if (config.noEdit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic structure here mirrors the package.json modification section well. Good consistency. One minor suggestion: consider adding a check for the existence of jsr.json earlier (perhaps right after checking config.jsr) with a more helpful error message like "jsr: true requires a jsr.json file to exist" to make it clearer to users what the issue is.

src/main.ts Outdated
conditions?: Record<string, "esm" | "cjs" | "src">;
tsconfig?: string; // optional path to tsconfig.json file
noEdit?: boolean;
jsr?: boolean;
Copy link
Owner

Choose a reason for hiding this comment

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

cc @pullfrog drop this. zshy should write to jsr.json if it exists. the existence of the file is the way to enable this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed the jsr config option. Now zshy automatically detects if jsr.json exists and writes exports to it if present.

Pullfrog  | Triggered by Pullfrog | Using Claude CodeView workflow run𝕏

Copy link
Contributor

Choose a reason for hiding this comment

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

Done! Removed the jsr config option. Now zshy automatically detects and writes to jsr.json if it exists in the project.

Pullfrog  | Triggered by Pullfrog | Using Claude CodeView workflow run𝕏

@pullfrog
Copy link
Contributor

pullfrog bot commented Dec 9, 2025

✅ Review feedback addressed

Changes made:

  • Removed jsr?: boolean from RawConfig and NormalizedConfig interfaces
  • Updated the jsr.json export writing logic to automatically detect file existence using findConfigPath()
  • If jsr.json exists in the project, zshy will automatically write exports to it without requiring any configuration

Implementation approach:
The code now uses a try/catch block to check if jsr.json exists. If the file is found, exports are written to it. If not found, the section is silently skipped.

Committed and pushed to the PR branch.

Pullfrog  | Triggered by Pullfrog | Using Claude CodeView workflow run𝕏

@pullfrog
Copy link
Contributor

pullfrog bot commented Dec 9, 2025

Leaping into action...

Pullfrog  | Triggered by Pullfrog | Using Claude CodeView workflow run

@pullfrog
Copy link
Contributor

pullfrog bot commented Dec 9, 2025

✅ Review feedback addressed!

Changes made:

  • Removed the jsr?: boolean config option from both RawConfig and NormalizedConfig interfaces
  • Removed the config.jsr ??= false normalization logic
  • Changed jsr.json handling to use automatic detection: zshy now scans the project hierarchy for jsr.json and automatically writes exports to it if found
  • No explicit configuration needed - the presence of jsr.json is all that's required to enable JSR support

The implementation now works exactly as requested: zshy automatically writes to jsr.json when it exists, without requiring any configuration flag.

Pullfrog  | Triggered by Pullfrog | Using Claude CodeView workflow run𝕏

pullfrog and others added 3 commits December 9, 2025 01:04
Changed the implementation to automatically detect if jsr.json exists in the
project rather than requiring a `jsr: true` config option. This simplifies
the API - users just need to create a jsr.json file to enable JSR export
writing.

- Removed `jsr` property from RawConfig and NormalizedConfig interfaces
- Updated jsr.json section to use try/catch with findConfigPath()
- If jsr.json exists, zshy will automatically write exports to it
@DallasHoff
Copy link
Contributor Author

Updated to address things missed by @pullfrog

@pullfrog
Copy link
Contributor

pullfrog bot commented Dec 9, 2025

Review complete

The author successfully addressed the owner's feedback. The implementation now correctly:

  • ✅ Removed the jsr config option from both RawConfig and NormalizedConfig interfaces
  • ✅ Uses automatic detection - zshy writes to jsr.json if it exists, without any configuration needed
  • ✅ Uses findConfigPath() which now returns null when a file doesn't exist
  • ✅ Package.json has proper null checking with an error message
  • ✅ jsr.json has proper null checking that silently skips if not found

However, I found two minor cleanup issues that were missed:

1. Test description outdated (line 176 of test/zshy.test.ts):

- it("should copy exports to jsr.json when jsr is true", () => {
+ it("should copy exports to jsr.json when jsr.json exists", () => {

2. Unrelated style changes in README (lines 695 & 705):
The PR includes unrelated changes converting single quotes to double quotes in code examples. These should be reverted to keep the PR focused:

- console.log("hello");
+ console.log('hello');

I've created a commit locally with these fixes, but I cannot push to your fork. Here's the complete patch you can apply:

diff --git a/README.md b/README.md
index 8da1472..4f7c834 100644
--- a/README.md
+++ b/README.md
@@ -692,7 +692,7 @@ To learn more, read the ["Masquerading as CJS"](https://github.com/arethetypeswr
 
 ```ts
 function hello() {
-  console.log("hello");
+  console.log('hello');
 }
 
 export default hello;
@@ -702,7 +702,7 @@ export default hello;
 
 ```ts
 function hello() {
-  console.log("hello");
+  console.log('hello');
 }
 exports.default = hello;
 module.exports = exports.default;
diff --git a/test/zshy.test.ts b/test/zshy.test.ts
index a7aec22..467cb38 100644
--- a/test/zshy.test.ts
+++ b/test/zshy.test.ts
@@ -173,7 +173,7 @@ describe("zshy with different tsconfig configurations", () => {
     expect(snapshot).toMatchSnapshot();
   });
 
-  it("should copy exports to jsr.json when jsr is true", () => {
+  it("should copy exports to jsr.json when jsr.json exists", () => {
     const snapshot = runZshyWithTsconfig("tsconfig.json", {
       dryRun: false,
       cwd: process.cwd() + "/test/jsr",

You can apply this patch with git apply or make the changes manually. Otherwise, the implementation looks great! 🎉

Pullfrog  | Triggered by Pullfrog | Using Claude CodeView workflow run𝕏

@DallasHoff
Copy link
Contributor Author

@pullfrog I have updated the test name. The quotation mark changes were made automatically by Biome for consistency, so you don't need to keep pointing them out.

@pullfrog
Copy link
Contributor

pullfrog bot commented Dec 9, 2025

Thank you for the update! I can confirm:

✅ The test name has been updated to "should copy exports to jsr.json when jsr.json exists" (line 176)

Understood regarding the quotation mark changes in the README - since they're from Biome's automatic formatting for consistency, I won't flag them in future reviews. The PR looks good now!

Pullfrog  | Triggered by Pullfrog | Using Claude CodeView workflow run𝕏

@colinhacks colinhacks merged commit 7c072d4 into colinhacks:main Dec 30, 2025
1 check passed
@colinhacks
Copy link
Owner

Landed in 0.7.0. Thanks!

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.

3 participants