feature: SDK ESM version

safe-global / safe-apps-sdk

Client-side SDKs to create third-party Safe Apps

https://safe.global

MIT License

242 stars 115 forks source link

feature: SDK ESM version #512

Closed VladimirMikulic closed 11 months ago

VladimirMikulic commented 11 months ago

In this PR, I added support for generating ESM version of the SDK.

Previously, the SDK was only distributed in CJS version which isn't optimized to be used in the browser and can hardly be tree shaken. Furthermore, dependencies shared with the app can't be deduplicated which results in bundling the same dependency twice.

Here's an example of viem (50kb) being bundled twice in our app at work. (CJS and ESM version of viem) Notice that @safe-global package size is 57kb.

Screenshot_20230817_010217

Here's the bundle with safe-apps-sdk in ESM version. Notice that @safe-global package size is only 4kb (93% decrease)! That's because viem package was deduplicated and it's only being included once (ESM) as opposed to twice (CJS and ESM).

Screenshot_20230817_024032

Thanks!

changeset-bot[bot] commented 11 months ago

🦋 Changeset detected

Latest commit: 0af0d6e03d1ebd9d1430d0c09c6e14a095c76904

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

| Name | Type | | -------------------------------- | ----- | | @safe-global/safe-apps-sdk | Major | | @safe-global/safe-apps-provider | Patch | | @safe-global/safe-apps-react-sdk | Patch | | @safe-global/safe-apps-test-app | Patch | | @safe-global/safe-apps-web3modal | Major |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

mmv08 commented 11 months ago

You're the MVP @VladimirMikulic, will ping the team to review

VladimirMikulic commented 11 months ago

Much appreciated @mmv08! Thanks for the fast reply as always!

yagopv commented 11 months ago

Hey @VladimirMikulic!!, the PR should go to the development branch

yagopv commented 11 months ago

dist folder should be pushed as well

yagopv commented 11 months ago

Hey @dasanra , some other things to take into account

If we decide to merge this PR we should release first a prerelease version and do some quick tests with common solutions as vite or bundlers. Test both cjs and esm approaches and ensure all the methods in the sdk are working fine and not affected by the TS5 migration. By the way, testing locally with our safe-apps-test-app is working and the size is decreased as commented:
As we are migrating to TS5 we should add a ticket to migrate the decorator we are using for the permissions system (under experimental flag) as now they are production ready but not compatible with the "old" experimental ones
Removing file extension would be nice but seems to be something required unless we use bundlers. We should add a ticket to do it with the other packages for making the monorepo cohesive
This solution combines ESM and CJS. For me is the correct one as we want to be retrocompatibles. More info here

VladimirMikulic commented 11 months ago

Appreciate the review @yagopv. I added dist folder.

yagopv commented 11 months ago

Hey @VladimirMikulic thanks for this PR. Really nice

We released -next.0 versions (prereleases) for our SDK packages so if you can test if this is working as expected for your project would be helpful. Thanks

We are going to test a little bit this prerelease and then publish the definitive packages