Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR transitions the project from a dual CommonJS/ESM build setup to ESM-only, representing a breaking change that simplifies the build configuration and modernizes the package structure.
- Removes dual build system and related TypeScript configurations
- Updates package.json to specify "type": "module" and consolidates entry points
- Replaces CommonJS module detection pattern with ESM equivalent using import.meta.url
Reviewed Changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.test.json | Removed test-specific TypeScript configuration |
| tsconfig.mjs.json | Removed ESM-specific TypeScript configuration |
| tsconfig.cjs.json | Removed CommonJS-specific TypeScript configuration |
| src/index.ts | Updated module detection from require.main to import.meta.url pattern |
| package.json | Set module type to ESM, updated build scripts, and replaced ts-node with tsx |
| fixup.sh | Removed build script that created dual package.json files |
| .eslintrc | Minor formatting cleanup and removed mocha environment |
| .cfignore | Removed Cloud Foundry ignore file |
| .babelrc | Removed Babel configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
dopry
force-pushed
the
chore/esm
branch
2 times, most recently
from
September 20, 2025 03:58
3533cd3 to
8b040f6
Compare
dopry
force-pushed
the
chore/esm
branch
8 times, most recently
from
June 26, 2026 19:59
d27f64a to
f60fe95
Compare
Re-application of the original chore/esm work on top of the refactored
next branch (Phases 1-7 moved the build to tsup and split the HTTP layer,
so the old diff no longer applied).
- package.json: "type": "module"; single ESM export map (types + default);
drop the require/import conditional exports and the "module" field
- tsup: build esm format only (dist/index.js + dist/index.d.ts)
- tsconfig: module/moduleResolution nodenext + verbatimModuleSyntax
- add explicit .js extensions to all relative imports in src/ and test/
- convert type-only named imports to "import type" / inline type modifiers
- src/index.ts: replace the CJS require.main entry-point guard with an
import.meta.url check so "node dist/index.js" still runs the server
- add test/integration/esm-package.spec.ts verifying the built dist/ is
consumable from real Node ESM via the package exports map and emits no
empty side-effect imports
- docs: update module usage example and copilot-instructions for the
ESM-only build
BREAKING CHANGE: @dopry/pecans is now ESM-only. require('@dopry/pecans')
is no longer supported; use import (Node >= 22.12).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXcPSv6FFTPgD59T4KudM
Follows up on Copilot review: PecansRelease/PecansReleases (and other names used only in type positions) are classes, so tsc accepts plain imports, but with verbatimModuleSyntax they would stay in the emitted JS as runtime imports. Convert the type-only usages to import type to keep the runtime module graph minimal and cycle-free. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXcPSv6FFTPgD59T4KudM
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 69 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/index.ts:77
pathToFileURL(process.argv[1])will throw ifprocess.argv[1]is a relative path (common when starting vianode dist/index.js). That makes the “run directly” guard crash the process beforemain()can run.
Normalize process.argv[1] to an absolute file URL before comparing to import.meta.url (or use resolve() + pathToFileURL).
if (
process.argv[1] &&
import.meta.url === pathToFileURL(process.argv[1]).href
) {
main();
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 21, 2026
# [2.0.0-next.18](v2.0.0-next.17...v2.0.0-next.18) (2026-07-21) * feat!: unified ReleaseService resolution pipeline on discrete os/arch/pkg (Phase 7 PR B) ([#44](#44)) ([8a2f06a](8a2f06a)) ### Bug Fixes * **github:** default octokit to native fetch to prevent empty release lists ([#28](#28)) ([0f6eac6](0f6eac6)) * honor route params in downloads, dead code removal, small fixes (Phase 2) ([#31](#31)) ([9fd8f79](9fd8f79)) * semantic-release trusted publishing ([4f1ee88](4f1ee88)) * semantic-release trusted publishing ([#53](#53)) ([3d4d4ce](3d4d4ce)) * working generic refresh webhook; document raw as the backend-private asset slot (Phase 7 PR A) ([#43](#43)) ([23d1d9f](23d1d9f)) ### chore * esm only ([#24](#24)) ([1b8a546](1b8a546)) ### Features * dependency modernization — Express 5, octokit 22, remove UA autodetection (Phase 5) ([#41](#41)) ([8ea329a](8ea329a)) * modernize packaging and dev tooling (Phase 3) ([#32](#32)) ([3345ed7](3345ed7)) * recognize .msix / .msixbundle assets and 'msix' package format ([#46](#46)) ([4f848b9](4f848b9)), closes [#26](#26) * typed HTTP errors with router-scoped error handling (Phase 6) ([#42](#42)) ([e3b7b54](e3b7b54)) ### BREAKING CHANGES * @dopry/pecans is now ESM-only. require('@dopry/pecans') is no longer supported; use import (Node >= 22.12). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXcPSv6FFTPgD59T4KudM * refactor: import model types with import type in runtime modules Follows up on Copilot review: PecansRelease/PecansReleases (and other names used only in type positions) are classes, so tsc accepts plain imports, but with verbatimModuleSyntax they would stay in the emitted JS as runtime imports. Convert the type-only usages to import type to keep the runtime module graph minimal and cycle-free. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zXcPSv6FFTPgD59T4KudM * Pecans no longer exposes a versions property. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM * feat!: remove the Versions and resolveReleaseAssetForVersion adapters The deprecation shims this PR introduced are dropped instead of carried to 3.0: route handlers and consumers resolve through ReleaseService / resolveAssetForRelease directly. The table-driven specs that pinned the legacy composite-id resolution semantics are migrated onto the pipeline (via platformToQuery) so the behavioral pins survive the adapter removal; unique Versions coverage moved into service.spec. Pre-existing deprecations (GitHubBackend, PecansSettings.timeout, PecansReleaseDTO.channel) keep their 3.0 schedule. * Versions, VersionFilterOpts, PlatformQuery, and resolveReleaseAssetForVersion are no longer exported. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM * getArchFromUserAgent, getOsFromUserAgent, and getPlatformFromUserAgent now take the internal UserAgentDetails type instead of express-useragent's Details, and getArchFromUserAgent defaults Windows and Linux to '64' (32-bit desktops are effectively extinct; the function is not used internally). pecans consumed exactly four booleans from the unmaintained express-useragent package; src/utils/userAgent.ts derives them from the User-Agent header directly, with mobile exclusions the old library handled via separate flags (iOS UAs contain 'like Mac OS X', Android UAs contain 'Linux'). The middleware attaches the same req.useragent shape. Unit specs cover the parser; the Phase 1 UA-driven download contract tests pass unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM * fix: validate update-route params through getStringParam consistently Review feedback: handleUpdateOSX truthiness-checked req.params directly but read values through getStringParam, and handleUpdateWin had no version guard at all; a missing tag would have produced a '>=undefined' range. Validate once through the helper and reuse the validated values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM * fix: exclude Macintosh+Mobile webview UAs from macOS detection Review feedback (partial): a Mobile token alongside Macintosh indicates an iPad-class webview masquerading as a Mac; genuine macOS browsers never send it. Fixture + test added. Note true iPadOS desktop-mode UAs are byte-identical to Mac Safari and undetectable by any parser. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM * fix: short-circuit dlfilename when the filename param is absent Review feedback: an undefined filename passed into queryReleases matches every release (the predicate treats undefined as no-filter), which would serve an arbitrary asset instead of a 404. Unreachable via the current route but guarded for consistency with the update handlers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM * feat: remove user-agent platform autodetection * selecting a platform is now the client's responsibility. GET / is no longer a download route, and the platform segment is required on /download, /download/version/:tag, and /download/channel/:channel (a missing platform returns 400). The user-agent parser, its middleware, and the getPlatformFromUserAgent / getArchFromUserAgent / getOsFromUserAgent helpers are removed. Autodetection only ever served bare browser links - Squirrel update clients and /dl/* always send explicit platforms - and reliable device detection is better handled client-side where UA Client Hints are available. Reverting this commit restores the feature wholesale if anyone misses it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM * fix: remove imports orphaned by the autodetection removal Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM * fix: validate tag ranges early in validateReqQueryTag Review feedback: validRange's result was discarded, so invalid tags only failed deep in release matching with a generic 'Invalid Range Specified' error. Invalid ranges now throw UnsupportedTagError at the parameter boundary ('latest' stays allowed), and the error message no longer says 'channel' for tags (copy-paste from UnsupportedChannelError). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM * build output moves from dist/cjs + dist/mjs to a tsup bundle (dist/index.js CJS, dist/index.mjs ESM). Deep imports into dist paths no longer resolve; all models (PecansRelease, PecansAsset, PecansReleases, ...) are now exported from the package root instead. - replace the dual-tsc + fixup.sh build with tsup (CJS + ESM + d.ts + sourcemaps, node22 target); consolidate four tsconfigs into one typecheck-only tsconfig.json - add a proper exports map with types for both module systems; verified with publint and arethetypeswrong (all green: node10/node16/bundler) - declare debug and qs as real dependencies - both are imported directly but were only present transitively via express, which broke the ESM bundle (inlined CJS require calls) - guard the run-directly check with typeof require so the ESM build is importable; node dist/index.js still starts the server - ts-node out of runtime dependencies; dev now runs tsx watch; start runs the compiled dist; drop nodemon - ESLint 9 flat config + prettier (replaces the stale mocha-era .eslintrc); fix the 19 findings it surfaced (unused imports/vars, case-block declarations, error causes, no-useless-assignment) - add explicit @types/node; add lint/format/typecheck scripts - package.json: type commonjs, sideEffects false, canonical repo url Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM * fix: rank .tar.gz assets by their full extension in resolveForVersion path.extname reports '.gz' for .tar.gz filenames, so the sort fallback ranked them at prefs.indexOf(-1) - ahead of every genuine preference - whenever .tgz and .tar.gz assets coexisted. Use getSupportedExt, which handles the double extension, matching the Phase 2 fix to PecansAsset.satisfiesExtensions. Regression test added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM * fix: throw Error from configure(), correct Listening typo Review feedback: the default switch case in configure() threw a raw string (no stack trace); the startup log said 'Lisening'. The test that pinned the typo is updated to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM * fix: throw on invalid os in getDownloadExtensionsByOs Review feedback: the switch had no default, so an invalid OperatingSystem cast in at runtime silently returned undefined against the declared SupportedFileExtension[] return type. Fail loudly instead; edge-case test updated to pin the throw. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes
@dopry/pecansan ESM-only package. Rebased onto the currentnext(Phases 1–7: tsup build, Express 5, HTTP route split, ReleaseService pipeline) as a single re-applied commit.Changes
"type": "module", single ESM export map (types+default), drops therequire/importconditional exports and themodulefield.esmonly →dist/index.js+dist/index.d.ts.module/moduleResolutionnodenext+verbatimModuleSyntaxso typecheck enforces real Node ESM resolution..jsextensions on all relative imports; type-only imports converted toimport type/ inlinetypemodifiers.require.main === moduleentry-point guard with animport.meta.urlvsprocess.argv[1]check, sonode dist/index.js(Docker CMD) still starts the server.test/integration/esm-package.spec.ts: buildsdist/and imports@dopry/pecansby name in a realnodesubprocess (through theexportsmap, exactly like a published consumer), and asserts no empty side-effect imports (import {} from ...) are emitted.docs/module.mdexample switched toimport; copilot-instructions updated for the ESM-only build.BREAKING CHANGE
require('@dopry/pecans')is no longer supported — consumers mustimportit (Node >= 22.12).