Skip to content

Conversation

@szymonrybczak
Copy link
Collaborator

Summary

Migrates to ESM.

Test plan

pnpm build should produce build in ESM format.

@szymonrybczak szymonrybczak requested a review from grabbou October 3, 2024 08:13
Copy link
Collaborator

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed some issues with the current state of the branch:

image
  1. d.ts files are generated per source file, while index.js seems to be bundled together. These should probably be the same, either per-file or bundled.
  2. There is no bin.js in create-app
  3. test-helpers is having dist in top-level folder, while create-app has it's own in packags/create-app. I think the second option is better if we want to run binaries, as we don't have to install deps in dist then.

@mdjastrzebski
Copy link
Collaborator

  1. __dirname does not seem to get resolved properly during build:
image

@szymonrybczak szymonrybczak force-pushed the chore/migrate-from-cjs-to-esm branch from e3d9756 to 3645000 Compare October 5, 2024 15:13
@mdjastrzebski
Copy link
Collaborator

I've done a round of checks and following things now work:

  • pnpm build and test are working fine
  • There is a bundled bin.js in packages/create-app/dist and it runs correctly

One piece that could be fixed is that while JS get's bundled, the d.ts are still geneated on a per-file basis:
image

The test-helpers packaged does not get build automatically (even with pn build = nx run-many -t build). But if you build it with nx build test-helpers then it is build in top-level dist/packages/test-helpers and JS are also generated on per-file basis (unlike create-app) 🤔
image

@szymonrybczak
Copy link
Collaborator Author

@mdjastrzebski after migration to tsc all things that you've mentioned should be resolved 👍

@szymonrybczak szymonrybczak merged commit 76ddbd1 into main Oct 8, 2024
1 check passed
@szymonrybczak szymonrybczak deleted the chore/migrate-from-cjs-to-esm branch October 8, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants