Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

perf: reduce size of published ganache bundle #798

Closed
wants to merge 9 commits into from

Conversation

honestbonsai
Copy link

@honestbonsai honestbonsai commented Feb 18, 2021

Resolves #744

This PR removes the native node modules from dist/cli, which reduces the total dist/cli size from 29M to 3.6M

TODO

  1. Test cli: node ./src/packages/ganache/dist/cli/ganache.min.js
  2. Test node
    // test.js
    const ganache = require("./src/packages/ganache/dist/node/ganache.min.js");
    console.log(ganache); // {server: ..., provider: ...}
    
  3. Test browser
    npm run build
    npm run docs.preview
    // Look at the interactive console, run some commands
    // const accounts = await provider.request({ method: "eth_accounts", params: []})
    // console.log(accounts)
    
  4. Clean up, make things easier to understand
  5. Consider removing cli exports not doing

@honestbonsai honestbonsai marked this pull request as ready for review February 22, 2021 16:59
Copy link
Member

@davidmurdoch davidmurdoch left a comment

Choose a reason for hiding this comment

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

Awesome work, @heyse! This is huge! (that was a bundle-size joke)

I've left some suggestions and comments. Let me know if you want to hop on zoom to review

src/packages/ganache/webpack/webpack.cli.config.ts Outdated Show resolved Hide resolved
@@ -34,7 +34,6 @@ const base: webpack.Configuration = {
output: {
filename: "ganache.min.js",
library: "Ganache",
libraryExport: "default",
Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts on if this change has an effect elsewhere?

Copy link
Author

Choose a reason for hiding this comment

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

The only change I noticed is that it manages to export the named exports in addition to the default export. I think if this is set to default, it only exports the default export.

Copy link
Author

@honestbonsai honestbonsai Feb 23, 2021

Choose a reason for hiding this comment

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

If we want to be really safe, I can isolate this change to just webpack.node.config

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok.

Can you run some smoke tests locally to ensure we can still import and require both the node and cli bundles?

basically:

// index.js
const ganache = require("{path-to-file}/ganache.min.js");
console.log(ganache) // should contain `provider` and `server` (`default` show up here, too)
// index.ts, with no/default tsconfig.json (i.e., `esModuleInterop` is set to `false`)
import ganache from "{path-to-file}/ganache.min.js";
console.log(ganache) // should contain `provider` and `server`

Copy link
Member

@davidmurdoch davidmurdoch Feb 24, 2021

Choose a reason for hiding this comment

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

important Note: I have NOT checked if this is how it behaved prior to your changes, but it's how I'd like it to behave, if possible. :-)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it really makes sense to have it exposed to the public interface at the level it is now. I'd really prefer if it wasn't exposed to users at all, but that'd likely be a lot more work.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can easily move serverDefaults from packages/core over to packages/cli; that way we won't need to expose it at all!

Copy link
Author

Choose a reason for hiding this comment

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

I wonder if we can easily move serverDefaults from packages/core over to packages/cli; that way we won't need to expose it at all!

I don't think this is possible as core uses it in src/packages/core/src/options/index.ts

Copy link
Author

Choose a reason for hiding this comment

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

important Note: I have NOT checked if this is how it behaved prior to your changes, but it's how I'd like it to behave, if possible. :-)

fyi this is how it is currently in develop

node test.js 
{ server: [Function: server], provider: [Function: provider] }

Copy link
Author

@honestbonsai honestbonsai Mar 2, 2021

Choose a reason for hiding this comment

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

After exported a named server and provider and patching the inferred type issue

node test.js 
{ serverDefaults: [Getter],
  server: [Function: server],
  provider: [Function: provider],
  default:
   { server: [Function: server], provider: [Function: provider] } }

src/packages/ganache/index.ts Outdated Show resolved Hide resolved
src/packages/cli/package.json Outdated Show resolved Hide resolved
@davidmurdoch davidmurdoch changed the title Feature/cli bundle slimming perf: reduce size of published ganache bundle Feb 22, 2021
Co-authored-by: David Murdoch <davidmurdoch@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

published ganache bundle is very large
2 participants