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

chore: build kits with cjs and esm targets #1049

Draft
wants to merge 21 commits into
base: development
Choose a base branch
from
Draft

Conversation

dasanra
Copy link
Collaborator

@dasanra dasanra commented Nov 15, 2024

What it solves

Continues #1047

How this PR fixes it

@dasanra dasanra requested review from yagopv and DaniSomoza November 15, 2024 14:14
@coveralls
Copy link

coveralls commented Nov 15, 2024

Pull Request Test Coverage Report for Build 12826671185

Details

  • 9 of 9 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 75.343%

Totals Coverage Status
Change from base Build 12811371480: 0.1%
Covered Lines: 800
Relevant Lines: 989

💛 - Coveralls

@frontendphil
Copy link

Hey folks! I wanted to give this a little bump because I'm working on the next thing, and once again, I'm seeing your code in a stack trace related to the whole CJS/ESM interop stuff. As always, If there is anything I can do to help push this forward, just let me know.

@DaniSomoza
Copy link
Contributor

Hi @frontendphil

We’ve made significant progress and, after running multiple tests, it seems to be working well overall.

However, there’s one issue related to CommonJS compatibility that we’re still trying to address.

Specifically, due to the way the Safe class from the protocol-kit is exported, users currently need to use .default when importing it in CommonJS:

const Safe = require('@safe-global/protocol-kit').default // TODO: remove .default

Safe.init({ key: 'value' })

We’d love to eliminate the need for .default to simplify integration for CommonJS, but we haven’t found a clean, simple solution without introducing extra complexity.

Thanks for your time and support.

@frontendphil
Copy link

Hey @DaniSomoza

Thanks for getting back to me. This probably won't be the answer you're looking for, but I need to ask anyway :) Would eliminating the default export (which is discouraged anyway) and replacing it with a named export be something you'd consider?

Then, instead of:

const Safe = require('@safe-global/protocol-kit').default

People would need to do:

const { Safe } = require('@safe-global/protocol-kit')

This would mean a breaking API change, but it may also be worth considering.

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.

[SDK Refactoring] ESM Modules
4 participants