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

feat: enable dynamic import of wallet libraries #100

Merged
merged 4 commits into from
Aug 6, 2023

Conversation

stefanofa
Copy link
Contributor

@stefanofa stefanofa commented Aug 1, 2023

Description

Prior to this change, each wallet library had to be imported statically, which greatly impacted the final build of a frontend due to the large size of these libraries.
With these changes, the possibility of dynamically importing these libraries is added in a backward compatible manner, and the developer can choose whether or not to opt for.

Specifically, the getDynamicClient parameter has been introduced. Users can incorporate this parameter into a provider's configuration. This parameter should be a function that returns a promise, which upon resolution, provides the corresponding wallet's client.

Currently, I haven't added any new unit tests since these would likely mock the implementation and thus, offer minimal benefit. Furthermore, considering the nature of the modification, the TypeScript static type verification inherent should suffice (even before this change, I do not think there was any testing of init methods).

To show the benefits of these changes, here is how much the frontend in nextjs that I used for testing was reduced in size:

BEFORE

screenshot-2023-08-02-00 45 52

AFTER

screenshot-2023-08-02-00 30 41

At present, the code executes the initialization of all providers internally via useInitializeProviders. However, to enhance library loading efficiency in the future, it would be advantageous to trigger init methods:

  • When a user selects the wallet they wish to connect with (i.e. the init call is delayed until the first time connect is called on the specific client),
  • During reconnection, solely for wallets with prior connections (retrieved from local storage).

Checklist

Please, make sure to comply with the checklist below before expecting review

  • Code compiles correctly
  • All tests passing
  • Extended the README / documentation, if necessary

@stefanofa
Copy link
Contributor Author

I also added in the storybook Example a test with the dynamic import.
However, I have noticed that storybook has not worked for a while in this repository (~10 months), this is caused by conflicts in the various dependencies.

To fix it, delete the node_modules and run:

npx yarn-deduplicate --strategy=highest
yarn

If you are ok I can create a commit in this PR, or it can be done later in another PR.

@drichar
Copy link
Collaborator

drichar commented Aug 4, 2023

Appreciate you submitting this, @stefanofa! I'll give it a full review tomorrow and let you know if I have any feedback.

The reduction in bundle size is substantial, this is a huge improvement. I also like your suggestion of deferring client initialization until it's necessary.

If you'd like to submit the Storybook fix in another PR, I'll include both updates in the next release.

@drichar drichar changed the base branch from main to next August 4, 2023 05:37
Copy link
Collaborator

@drichar drichar left a comment

Choose a reason for hiding this comment

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

This is an excellent solution, and will be the recommended approach for apps that support dynamic imports.

I'll update the https://github.com/TxnLab/next-use-wallet example once this is published. Seeing a comparable first load bundle size reduction there as well.

Thank you so much for the contribution!

@drichar drichar merged commit 57f38f3 into TxnLab:next Aug 6, 2023
1 check passed
@stefanofa stefanofa deleted the feature/dynamic-import branch August 10, 2023 10:26
@stefanofa stefanofa mentioned this pull request Aug 10, 2023
2 tasks
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.

2 participants