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(cli): remove unused symbols from codegen #200

Merged
merged 2 commits into from Nov 28, 2023

Conversation

voliva
Copy link
Contributor

@voliva voliva commented Nov 26, 2023

My attempt at solving #198 through method [2a].

I've built a rollup plugin (which is compatible with vite) that given the path to a codegen folder, detects calls to createClient using the generated descriptors, traces their usages and removes the unused descriptors from the build.

With the current example of vite/src/main.ts, the minified code is shaved by ~50kB:

minified
dist/assets/index-9b9328ce.js                       140.46 kB │ gzip:    53.27 kB
dist/assets/index-91969805.js                        89.86 kB │ gzip:    31.66 kB

Resulting built code without the plugin:

Screenshot 2023-11-26 at 10 40 36

Resulting built code with the plugin:

Screenshot 2023-11-26 at 10 42 01

This plugin is able to trace many common cases:

import { createClient } from "@polkadot-api/client";
import testDescriptors from "./codegen/test";

function transfer(alexa: Account, billy: Account, amount: bigint) {
  // Will be tracked even if `client` is defined further down in the AST
  client.tx.Balances.transfer_keep_alive.submit$(/* ... */);
}

const client = createClient(chain.connect, testDescriptors);

const { tx: transactions } = client;

// Will also be tracked even after the rename
const data = transaction.Slots.clear_all_leases.getCallData(0);

// Any file importing `transactions` and using it will also be tracked
export { transactions };

And it also covers the minimum for source map support. This can be used as an initial version where it covers many of the happy paths. There are still a few tweaks to improve and some challenges to solve:

  • Computed properties client.tx.Balances[method], or any thing that has to get solved in runtime. Currently it ignores it, but I would suggest bailing out from removing any symbol when detecting such cases.
  • Passing tracking variables through function arguments instead of directly referencing them. This could technically be solved by finding out the call arguments, with a few edge cases, but it will get more complex and tricky if the function is defined in a separate file.
function transfer(
  selectedClient: typeof client,
  alexa: Account,
  billy: Account,
  amount: bigint
) {
  // Currently won't be tracked :(
  selectedClient.tx.Balances.transfer_keep_alive.submit$(/* ... */);
}
const client = createClient(chain.connect, testDescriptors);
transfer(client, alexa, billy, 30n);
  • Currently destructuring is only supported one level deep, it should support going further down: const { tx: { Balances }} = client. There are other assignment patterns defined in the AST types that need to be handled as well.
  • Currently source maps just link the original code with all the symbols to the optimised code as a block, but doesn't go into the detail of which variables come exactly from where. It's just a technicality, but this is enough to let the consumer debug through the code.

I've added this as a new package rollup-plugin-descriptor-treeshake. I'm happy to change anything you might need, or if you think you want to use a different approach altoghether it's also fine; I just found this feature request interesting :).

@josepot
Copy link
Member

josepot commented Nov 26, 2023

🌟 This is absolutely fantastic work! I'm thrilled to see this progress and truly appreciate the effort you've put into this plugin. It's important to remember that this is a "nice to have" feature, so it's perfectly okay if it doesn't cover every edge case. Developers using @polkadot-api/client have several options, including:

  • Accepting a slightly larger bundle size in their dApps.
  • Maintaining a manual list of the chain interactions they need.
  • Adapting their coding patterns to align with this plugin's capabilities.

Now, let me share some thoughts on the specific points you raised:

  1. Computed Properties (client.tx.Balances[method]): Your approach of ignoring runtime-resolved properties seems sensible. In cases where we have something like client.tx.Balances[method], ideally, the plugin should retain all tx.Balances.* descriptors and trim the others. Similarly, for client.tx[palletName][callName], keeping all tx descriptors while trimming the rest would be ideal.

  2. Passing Tracking Variables Through Function Arguments: It's disappointing that this poses a challenge, but I understand the complexity. I had hoped it would be more straightforward, but I can totally see that it's trickier than I thought.

  3. Destructuring Support: While deeper destructuring like const { tx: { Balances }} = client might be uncommon, it's a valid point. However, if it proves too complex to implement, we can simply document this as a pattern that the plugin does not support for now.

  4. Source Maps: Your handling of source maps sounds completely adequate. It's more important that developers can debug through the code, rather than having a detailed mapping of every variable.

I can't express enough how grateful I am for your contribution. It's community efforts like yours that drive this project forward. Thank you so much for dedicating your time and skills to help improve our project. 🙏

@voliva
Copy link
Contributor Author

voliva commented Nov 27, 2023

Thanks for the feedback!

I have added a commit to handle nested destructuring, as it was rather simple to add.

@josepot
Copy link
Member

josepot commented Nov 28, 2023

Thanks for the feedback!

I have added a commit to handle nested destructuring, as it was rather simple to add.

This is awesome! Let's get this merged, for sure! We can take it from here, since the most difficult work has already been taken care off.

It would be very nice if you could take some time to look into the "Passing tracking variables through function arguments" edge-case 🙏

A tip proposal for this contribution has been submitted (using @polkadot-api/client in a tinny dApp bundled with this plugin 😄. I'm serious! )

Thanks again!

@josepot josepot self-requested a review November 28, 2023 00:34
Copy link
Member

@josepot josepot left a comment

Choose a reason for hiding this comment

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

Thanks!

@josepot josepot added this pull request to the merge queue Nov 28, 2023
Merged via the queue into polkadot-api:main with commit 62606ec Nov 28, 2023
5 of 6 checks passed
@voliva voliva deleted the tree-shaker branch November 28, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants