[CDX-468] Add modern ESM build#455
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an ESM build output alongside the existing CommonJS + bundled browser build, and wires it into test and release workflows so the project can ship “modern” ESM artifacts (npm module entry + CDN .esm.js).
Changes:
- Add esbuild-based ESM outputs:
lib/esm/constructorio.js(for npm) anddist/esm/*.esm.js(for CDN). - Add a dedicated bundled-ESM test path that uses a test-only IIFE artifact for jsdom execution.
- Update CI/workflows to run bundled ESM tests and publish the ESM bundle to the CDN.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/src/utils/jsdom-global.js | Selects the appropriate bundle artifact for bundled tests (legacy vs ESM variant). |
| spec/src/modules/quizzes.js | Updates bundled quiz tests to load the correct artifact for the ESM variant. |
| scripts/bundle-esm.js | New script producing the CDN ESM bundle in dist/esm. |
| scripts/build-test-iife.js | New script producing a test-only IIFE bundle used for bundled ESM tests. |
| scripts/build-esm.js | New script producing the npm ESM entrypoint under lib/esm. |
| package.json | Adds module entry and hooks ESM builds into postcompile/postbundle; adds bundled-ESM test scripts. |
| .gitignore | Ignores dist/test-only/ artifacts. |
| .github/workflows/run-tests-bundled-esm.yml | New workflow running the bundled ESM test suite in CI. |
| .github/workflows/publish-cdn.yml | Publishes the new dist/esm artifact to S3 (versioned + latest). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -0,0 +1,30 @@ | |||
| const packageJSON = require('../package.json'); | |||
There was a problem hiding this comment.
Should this live in script/s or should it be top-level like the other bundle.js file? Or maybe we should move that one down 🤔
There was a problem hiding this comment.
Good catch, I think it makes sense to move the other one down 26d402c
Co-authored-by: Ahmad Mudaafi' <ahmad.mudaafi@constructor.io>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This PR adds a modern ESM build output (lib/esm/ and dist/esm/) alongside the existing CJS/IIFE bundles, updates CI to test it, and publishes the ESM bundle to the CDN. The approach is well-structured overall, but there are a few important issues to address.
Inline comments: 7 discussions added
Overall Assessment:
No description provided.