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.

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

Add ESM build to client #3359

Merged
merged 27 commits into from
Apr 23, 2024
Merged

Conversation

acolytec3
Copy link
Contributor

@acolytec3 acolytec3 commented Apr 18, 2024

This replaces the current client build process with one that is ESM only.

Notable changes

  • Adds a "type":"module" to package.json
  • Adds a new ESM build step for npm run build
  • Removes all require(... usage in client
  • Revises imports of current CommonJS imports to use default imports and then destructuring as needed
  • Removes fs-extra dependency since not needed (and was causing issues with the ESM build)
  • Adds some typing in bin/cli.ts for the servers bit

TODOs:

  • Completely remove the now non-functional CJS build

@acolytec3 acolytec3 marked this pull request as draft April 18, 2024 16:49
Copy link

codecov bot commented Apr 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.73%. Comparing base (61acbd3) to head (6f3324c).

❗ Current head 6f3324c differs from pull request most recent head 34fa97c. Consider uploading reports for the commit 34fa97c to get more accurate results

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
client 84.73% <100.00%> (?)
tx ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

Some initial comments

Dockerfile Outdated Show resolved Hide resolved
packages/client/bin/cli.ts Show resolved Hide resolved
packages/client/src/util/rpc.ts Outdated Show resolved Hide resolved
@@ -176,7 +176,7 @@ export function startRPCServers(client: EthereumClient, args: RPCArgs) {
rpcDebug,
rpcDebugVerbose,
logger,
})
}) //@ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary? Can we just typecast instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was an earlier left over. Looks like we don't need it at all. Will remove.

gabrocheleau
gabrocheleau previously approved these changes Apr 23, 2024
Copy link
Contributor

@gabrocheleau gabrocheleau left a comment

Choose a reason for hiding this comment

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

Left a couple of questions/comments. Looks great

@gabrocheleau gabrocheleau marked this pull request as ready for review April 23, 2024 01:16
@acolytec3 acolytec3 merged commit 6b24349 into master Apr 23, 2024
34 checks passed
@acolytec3 acolytec3 deleted the make-client-ESM-for-the-very-first-time branch April 23, 2024 14:06
This was referenced Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants