Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(build): Fix ESM output (#7357) #7358

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

fix(build): Fix ESM output (#7357) #7358

wants to merge 14 commits into from

Conversation

fgnass
Copy link

@fgnass fgnass commented Dec 23, 2024

Summary

This PR fixes two related issues discovered while working on ESM builds:

  1. ESM builds incorrectly output CommonJS format despite being in dist/esm directories. This happens because Babel's preset-env transforms ES modules to CommonJS by default. The issue can be seen in decap-cms-core's ESM build.

  2. Build dependencies are not properly configured in Nx, causing builds to fail with empty cache as packages are built in parallel without respecting dependencies.

Changes:

  • Configure Babel to preserve ES modules by setting modules: false for ESM builds
  • Add proper build dependencies in nx.json to ensure correct build order
  • Remove direct imports from dist/esm in favor of proper package entry points

Fixes #7357

Test plan

  1. Clean the workspace:
bash
npm run clean
  1. Build directly with nx (this would fail before the fix):
NX_NO_CLOUD=true npx nx run decap-cms:build
  1. Verify ESM output format in any package's dist/esm/index.js:
// Should see ES module syntax:
export default DecapCmsCore;
// Instead of CommonJS:
exports.default = exports.DecapCmsCore = void 0;

Checklist

A picture of a cute animal
xmas-cat

@fgnass
fix: leave modules untouched in ESM build
ea589aa 
This prevents Babel from converting the ESM module syntax to CommonJS.
@fgnass
fix: include clearing the Nx cache in the clean script
a4158d0 
Otherwise a "clean" build will still use old artefacts from the cache.
@fgnass
fix: don't build dependant packages in parallel
7a07e31 
This ensures that any local dependencies are built first.
@fgnass
fix: don't import from dist/esm
eb060b1 
Thanks to the other changes in this PR, we can now safely import from the decap-cms-app.
@fgnass fgnass requested a review from a team as a code owner December 23, 2024 12:47
@martinjagodic martinjagodic requested review from demshy and removed request for a team December 23, 2024 15:24
@fgnass
fix(ci): use actions/setup-node cache option
2a00de4 
This will hopefully fix the error we get on Windows during `npm install`
@fgnass
chore(ci): fix typo and output matrix.node-version
e37891a
@fgnass
Copy link
Author

fgnass commented Dec 24, 2024

I noticed that the GitHub action is failing during npm install on Windows, which seems totally unrelated to the changes made by this PR. I added two further commits, which will hopefully resolve the build issue by enabling the cache option of actions/setup-node@v3 and using npm ci instead of npm install. Have you experienced similar issues with other PRs or this happening for the first time?

@martinjagodic
Copy link
Member

@fgnass Yes, sometimes the tests fail for no reason, so we must re-run them. Thanks for the fix!

@fgnass
Copy link
Author

fgnass commented Dec 24, 2024

Okay, the installation step now seems to work reliably, but the e2e tests failed, I guess because the files in dev-test/dist/ were missing or not correctly transferred from the build container to the one running the cypress tests. I simplified the workflow by running everything in the same container, making sure that the e2e test run only once (ubuntu / node 20.x) and not for every combination in the matrix.

@martinjagodic
Copy link
Member

@fgnass ubuntu + node 20 is still failing

@fgnass
Copy link
Author

fgnass commented Dec 24, 2024

I'll look into it and try to reproduce the error in my forked repo. Stay tuned.

@fgnass
Copy link
Author

fgnass commented Dec 25, 2024

Good news: The workflow did run successfully in my forked repo: https://github.com/fgnass/decap-cms/actions/runs/12483114867/job/34838341976

I'm curious if it will succeed here too!

@dragons-library
Copy link

FWIW, I was looking at this from the decap-cms-app perspective earlier, and it currently doesn't declare it's esm export in decap-cms-app/package.json:

"module": "dist/esm/index.js",

I think it should - then you wouldn't have to update your decap-cms-app import paths.

Also, somewhat unrelated, but decap-cms-app/package.json doesn't correctly declare a dependency on decap-cms-backend-gitea, which it should for the standalone esm module to be functional:

"decap-cms-backend-gitea": "^3.1.5",

Thanks for the work on this PR, it's much more extensive than what I had explored so far.

@fgnass
Copy link
Author

fgnass commented Dec 27, 2024

@dragons-library Good catch! I added the module field and the missing dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@demshy demshy Awaiting requested review from demshy

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ESM builds output CommonJS format despite being in dist/esm
3 participants
@fgnass @martinjagodic @dragons-library