fix: add "type": "module" + rename CJS bundle to .cjs (fixes #233) - #237
Merged
Merged
Conversation
) @dagrejs/graphlib points exports.import at dist/graphlib.esm.js (ESM syntax) but has no "type": "module", so runtimes that don't auto-detect ESM (tsx, Node without --experimental-detect-module) treat that .js file as CommonJS. Named imports then resolve to undefined and `new Graph()` throws TypeError. Add "type": "module" so the ESM bundle is interpreted as ESM, and rename the CJS bundle from dist/graphlib.cjs.js to dist/graphlib.cjs — the .cjs extension is pinned to CommonJS regardless of package type, so `require()` keeps working. A naive "Option A" (type: module without the .cjs rename) would break require() with ERR_REQUIRE_ESM, since the CJS bundle is a .js file using module.exports. The IIFE browser bundles (dist/graphlib.js, dist/graphlib.min.js) stay .js: they are loaded via <script>, are not in the Node exports map, and are unaffected by "type": "module". Adds test/dist-exports.test.ts, which exercises the BUILT dist via real runtimes (tsx for ESM, node for CJS) by importing it at an absolute path — test/bundle-test.ts imports source via ts-jest and cannot catch a dist-format bug. The ESM case fails on the current code (only `default` is exported) and passes after this change; the CJS case stays green throughout. Closes dagrejs#233. Co-Authored-By: Claude <noreply@anthropic.com>
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
3 participants
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.
Problem
@dagrejs/graphlibpointsexports.importatdist/graphlib.esm.js(ESM syntax:export { Graph, alg, json, version }), but the package has no"type": "module". Runtimes that don't auto-detect ESM syntax in.jsfiles — e.g.tsx, or Node without--experimental-detect-module— treat that ESM bundle as CommonJS. The named exports never materialize:import * as graphlibyields only{ default },Graph/alg/jsonareundefined, andnew Graph()throwsTypeError. Reported in #233.Fix — "Option A", done correctly
Add
"type": "module"topackage.jsonso Node treats.jsfiles as ESM, which makes the existingdist/graphlib.esm.jsresolve its named exports correctly.The required companion — why naive Option A breaks
require(): the CJS bundle is a.jsfile whose body usesmodule.exports. Adding"type": "module"makes Node treat every.jsfile as ESM, sorequire('@dagrejs/graphlib')would start throwingERR_REQUIRE_ESM. To keeprequire()working, the CJS bundle is renameddist/graphlib.cjs.js→dist/graphlib.cjs: the.cjsextension is pinned to CommonJS regardless of the packagetypefield. This PR does both —type: moduleand the.cjsrename — so neitherimportnorrequireregresses.Files
package.json— add"type": "module";mainandexports.".".require→dist/graphlib.cjs. (moduleandexports.".".importstay ondist/graphlib.esm.js, now correctly ESM.)build.ts— CJS esbuildoutfile→dist/graphlib.cjs(sourcemap follows asgraphlib.cjs.map).dist/— rebuilt:graphlib.cjs+graphlib.cjs.mapreplacegraphlib.cjs.js+.map; the ESM and IIFE bundles are unchanged.Makefile—BUILD_FILESupdated to the new CJS name (it referenced the old.cjs.js).IIFE browser bundles are unaffected
dist/graphlib.jsanddist/graphlib.min.jsstay.js. They are loaded via<script>, are not in the Nodeexportsmap, and"type": "module"does not affect browser script-tag loading.Regression test
test/bundle-test.tsimports../index(source) via ts-jest, so it cannot catch a published-dist-format bug. This PR addstest/dist-exports.test.ts, which exercises the built dist through real runtimes by importing it at an absolute path — the exact resolution that reads thetypefield from the dist file's nearestpackage.json:.mtsimportsdist/graphlib.esm.jsand assertsGraph/alg/json/versionare the expected types andnew Graph()works. Fails on the current code (onlydefaultis exported →SyntaxError: does not provide an export named 'Graph'); passes after this change..cjsrequire()sdist/graphlib.cjsand assertsnew Graph()works. Guards the CJS regression that a naive Option A would introduce. Stays green before and after.Both pass under the full jest suite.
Validation
require('@dagrejs/graphlib')under node and a namedimportunder tsx both work via theexportsmap.npm test(jest, 258 tests) green;npm run lintclean;npm run buildgreen.As requested in #233 (comment) — "If you already have the fix, open a pull request and I'll merge it. I'm a fan of Option A."
Closes #233.
🤖 Generated with Claude Code