Add support for package.json5 format#191
Conversation
src/install-pnpm/run.ts
Outdated
| ? YAML.parse(content, { merge: true }) | ||
| : JSON.parse(content) | ||
| : JSON5.parse(content) |
There was a problem hiding this comment.
You should change the logic into the following:
- If the file name ends with
.yaml, useYAML. - If the file name ends with
.json5, useJSON5. - Otherwise, use
JSON.
IIRC, pnpm only uses JSON5 for package.json5.
There was a problem hiding this comment.
@KSXGitHub
Thank you for the review.
Fixed in be2217e.
Co-authored-by: Khải <hvksmr1996@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds support for reading the packageManager field from package.json5, aligning this GitHub Action’s manifest parsing behavior with pnpm’s JSON5 support.
Changes:
- Add
json5as a dependency and lock it inpnpm-lock.yaml. - Extend manifest parsing to handle
.json5files when readingpackageManager/devEngines. - Update README to document
package.json5as a supported manifest format.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/install-pnpm/run.ts | Adds JSON5 parsing branch when reading the workspace manifest |
| README.md | Documents package.json5 support for package_json_file |
| package.json | Adds json5 dependency |
| pnpm-lock.yaml | Locks json5 dependency version and integrity |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@types/expand-tilde": "^2.0.2", | ||
| "@types/node": "^22.0.0", | ||
| "expand-tilde": "^2.0.2", | ||
| "json5": "^2.2.3", | ||
| "yaml": "^2.3.4", | ||
| "zod": "^3.22.4" |
There was a problem hiding this comment.
dist/index.js is the action entrypoint (action.yml), and the repo's PR workflow rebuilds and checks that dist/index.js is committed. Since this change adds a new runtime dependency (json5) and modifies parsing logic, you need to run pnpm run build and commit the updated dist/index.js; otherwise CI will fail and the published action won't include the change.
| const content = readFileSync(path.join(GITHUB_WORKSPACE, packageJsonFile), 'utf8'); | ||
| const manifest = packageJsonFile.endsWith(".yaml") | ||
| ? parseYaml(content, { merge: true }) | ||
| : packageJsonFile.endsWith(".json5") | ||
| ? JSON5.parse(content) | ||
| : JSON.parse(content) | ||
| packageManager = manifest.packageManager |
There was a problem hiding this comment.
PR description says non-YAML manifests are parsed with JSON5.parse() (which would cover both .json and .json5), but the implementation only uses JSON5.parse() for .json5 and still uses JSON.parse() for .json. Either update the description or simplify the logic to use JSON5.parse() for all non-YAML files (since JSON5 is a superset of JSON).
| const manifest = packageJsonFile.endsWith(".yaml") | ||
| ? parseYaml(content, { merge: true }) | ||
| : packageJsonFile.endsWith(".json5") | ||
| ? JSON5.parse(content) | ||
| : JSON.parse(content) |
There was a problem hiding this comment.
The nested ternary for selecting the manifest parser is getting hard to read/extend with multiple formats. Consider refactoring to a small helper (e.g., based on path.extname(packageJsonFile) or a switch) so future additions like .yml don’t further degrade readability.
| ### `package_json_file` | ||
|
|
||
| **Optional** (_type:_ `string`, _default:_ `package.json`) File path to the `package.json`/[`package.yaml`](https://github.com/pnpm/pnpm/pull/1799) to read "packageManager" configuration. | ||
| **Optional** (_type:_ `string`, _default:_ `package.json`) File path to the `package.json`/[`package.yaml`/`package.json5`](https://github.com/pnpm/pnpm/pull/1799) to read "packageManager" configuration. | ||
|
|
There was a problem hiding this comment.
README documents package.json5 support here, but action.yml’s package_json_file input description still only mentions package.json. To keep the Marketplace/action docs consistent, please update action.yml to mention package.yaml and package.json5 as supported formats too.
Summary
This PR adds support for
package.json5format when reading thepackageManagerfield, making this action consistent with pnpm's own support for JSON5.Motivation
According to pnpm's documentation:
This action already supports
package.jsonandpackage.yaml, but notpackage.json5. This PR adds the missing support to be consistent with pnpm itself.Changes
json5package as a dependencyJSON5.parse()instead ofJSON.parse()for non-YAML filespackage.json5support