Fix/js types #2594
Fix/js types #2594
Conversation
…truct#1448; chore: added import validation test coverage
…truct#1448; chore: added import validation test coverage
alixander
left a comment
alixander
left a comment
There was a problem hiding this comment.
Thank for this PR! Could you bump the version in package.json too?
ci/release/changelogs/next.md
Outdated
| @@ -1,5 +1,6 @@ | |||
| #### Features 🚀 | |||
|
|
|||
| - Fixed ESM and CJS builds of d2.js [#2286](https://github.com/terrastruct/d2/issues/2286) + [#1448](https://github.com/terrastruct/d2/issues/1448) | |||
There was a problem hiding this comment.
There's a separate changelog for d2js. Hasn't been updated in a bit, but that's where it should go: https://github.com/terrastruct/d2/blob/master/d2js/js/CHANGELOG.md
d2js/js/package.json
Outdated
| "test:unit": "bun test test/unit", | ||
| "test:integration": "bun test test/integration", | ||
| "test:all": "bun run test && bun run test:integration", | ||
| "test:all": "bun run test", |
There was a problem hiding this comment.
👍 , though I think it's just bun test now.
▶ bun run test
error: "/bin/test" exited with code 1
note: a package.json script "test" was not found
d2js/js/package.json
Outdated
| "devDependencies": { | ||
| "bun": "latest" | ||
| "bun": "latest", | ||
| "typescript": "^5.9.2" |
| test("can require main entry point without error", () => { | ||
| expect(() => { | ||
| const module = require("../../dist/node-cjs/index.js"); | ||
| expect(module).toBeDefined(); | ||
| expect(module.D2).toBeDefined(); | ||
| expect(typeof module.D2).toBe("function"); | ||
| }).not.toThrow(); | ||
| }); | ||
|
|
||
| test("worker module file exists", () => { | ||
| const fs = require("fs"); | ||
| const path = require("path"); | ||
| const workerPath = path.resolve(__dirname, "../../dist/node-cjs/worker.js"); | ||
| expect(fs.existsSync(workerPath)).toBe(true); | ||
| }); | ||
|
|
||
| test("exported D2 class is constructable", () => { | ||
| const { D2 } = require("../../dist/node-cjs/index.js"); | ||
| expect(() => new D2()).not.toThrow(); | ||
| }); | ||
|
|
||
| test("module exports match expected structure", () => { | ||
| const module = require("../../dist/node-cjs/index.js"); | ||
| expect(module).toHaveProperty("D2"); | ||
| expect(typeof module.D2).toBe("function"); | ||
| }); | ||
|
|
||
| test("can access both named and default exports", () => { | ||
| const module = require("../../dist/node-cjs/index.js"); | ||
| const { D2 } = require("../../dist/node-cjs/index.js"); | ||
|
|
||
| expect(module.D2).toBe(D2); | ||
| expect(module.D2).toBeDefined(); | ||
| }); | ||
|
|
||
| test("can compile a diagram", async () => { |
There was a problem hiding this comment.
this seems to have redundancy. how many of these are actually necessary (was failing before) if the "can compile a diagram" test was passing?
There was a problem hiding this comment.
(same reply to the esm test changes)
|
|
||
| describe("D2 CJS Integration", () => { | ||
| test("can require and use CJS build", async () => { | ||
| test("can require main entry point without error", () => { |
There was a problem hiding this comment.
these tests are still importing from the whole path, is there a way to test that it matches what the module/main path is defined as? e.g. require(d2js) and it resolves to the right one. otherwise it looks like there's no tests that exercise the change for module/main export paths.
|
Hi @mateothegreat is this ready for re-review? |
|
Hi @mateothegreat I plan to do a release of d2 this week and would love to get this in! If you're busy, would you mind if I commit changes? |
|
ya I got it baked, I'll push after a power nap 😉 |
2 participants
This is to fix the problems users are facing for importing EJS and CJS modules from d2js.
In addition to the update of
package.json, the test suite now adequately validates that both ESM and CJS builds can be imported without errors, exports are properly structured, and the main D2 class isconstructible.
Also, the paths for the outputs under
dist/were changed at some point fromdist/(esm|cjs)todist/(node-esm|node-cjs)- this is now fixed for future npmjs.com publishing.Changes
package.jsonand cleaned up + fixedexportsstructure.importand CJSrequirefunctionality.