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

Remove src directory and already-bundled dependencies from published package #3013

Closed
1000hz opened this issue Apr 10, 2023 · 6 comments · Fixed by #3191
Closed

Remove src directory and already-bundled dependencies from published package #3013

1000hz opened this issue Apr 10, 2023 · 6 comments · Fixed by #3191
Labels
breaking change Change that will result in breaking existing behavior enhancement New feature or request maintenance Maintenance task quick win Potentially easy/straightforward issue to tackle

Comments

@1000hz
Copy link
Contributor

1000hz commented Apr 10, 2023

We currently include the src directory in the published artifact, resulting in an extra 2.6 MB on disk.

Additionally, although we bundle the application into wrangler-dist/cli.js, we publish package.json still declaring dependencies on those bundled libraries, resulting in an additional 32 MB of packages getting installed even though they've already been included in the bundle.

In order to minimize installation time of wrangler, we should consider removing src from the published artifact and either moving bundled dependencies to devDependencies or publishing with a stripped down package.json that only includes within dependencies any externalized modules. This becomes more important as we start depending on larger external modules (e.g. workerd) and need to keep time-to-install within reasonable bounds.

@1000hz 1000hz added the enhancement New feature or request label Apr 10, 2023
@1000hz 1000hz added this to the Wrangler 3.0.0 milestone Apr 10, 2023
@1000hz 1000hz added the maintenance Maintenance task label Apr 10, 2023
@1000hz
Copy link
Contributor Author

1000hz commented Apr 15, 2023

On further inspection, it turns out we mostly already do only include externalized dependencies within our dependencies array. The only bundled dependencies we're unnecessarily installing seem to be nanoid and xxhash-wasm, which we should still move into devDependencies, though doing so will net us far less savings than I was hoping for (~250 KB on disk).

On the bright side, we're already doing the right thing, so GO TEAM! 🥳

Sorry for the false alarm, and h/t @petebacondarwin for calling attention to the quick savings we could gain by removing src.

@petebacondarwin
Copy link
Contributor

I think xxhash-wasm is still needed as a dependency for now. It is used to create a hash key for "Workers Sites" uploads. But once we move over to Pages based asset uploads, this can go.

@1000hz
Copy link
Contributor Author

1000hz commented Apr 17, 2023

oh, good to know! I just meant xxhash-wasm is currently bundled, not externalized, so we can move it over to devDependencies.
image

@1000hz 1000hz added the quick win Potentially easy/straightforward issue to tackle label Apr 17, 2023
@petebacondarwin
Copy link
Contributor

Thanks @1000hz for enlightening me about how xxhash-wasm has the WASM module inlined in the JS module! So indeed it is already bundled.

@GregBrimble
Copy link
Member

If we ship the JS API and want to provide declarationMaps, we'd probably want to include the src directory in the published package.

I'm a pretty big +1 to doing this! It can make using a package so much easier.

@lrapoport-cf lrapoport-cf added the breaking change Change that will result in breaking existing behavior label Apr 27, 2023
mrbbot added a commit that referenced this issue May 11, 2023
Prevents the following being included in the npm tarball:

- `src`: we may want to ship this if we shipped source maps, so
  users could click on internal lines in stack traces, but we don't
  ship source maps to npm, so these aren't needed.
- `miniflare-config-stubs`: these aren't used as of #633
  (https://github.com/cloudflare/workers-sdk/pull/633/files#diff-dd2c9d6fc577a3da1b7cf0b6b4a0705a1c71eb5d99c372d5bdda99476362f975L97-L102)
- `vendor`: this directory doesn't actually exist in the `wrangler`
   package directory, there is a root `vendor` directory, but files
   from that are copied to `wrangler-dist` during build

Closes #3013
mrbbot added a commit that referenced this issue May 12, 2023
Prevents the following being included in the npm tarball:

- `src`: we may want to ship this if we shipped source maps, so
  users could click on internal lines in stack traces, but we don't
  ship source maps to npm, so these aren't needed.
- `miniflare-config-stubs`: these aren't used as of #633
  (https://github.com/cloudflare/workers-sdk/pull/633/files#diff-dd2c9d6fc577a3da1b7cf0b6b4a0705a1c71eb5d99c372d5bdda99476362f975L97-L102)
- `vendor`: this directory doesn't actually exist in the `wrangler`
   package directory, there is a root `vendor` directory, but files
   from that are copied to `wrangler-dist` during build

Closes #3013
@mrbbot
Copy link
Contributor

mrbbot commented May 15, 2023

Closed by #3191

@mrbbot mrbbot closed this as completed May 15, 2023
penalosa pushed a commit that referenced this issue May 17, 2023
Prevents the following being included in the npm tarball:

- `src`: we may want to ship this if we shipped source maps, so
  users could click on internal lines in stack traces, but we don't
  ship source maps to npm, so these aren't needed.
- `miniflare-config-stubs`: these aren't used as of #633
  (https://github.com/cloudflare/workers-sdk/pull/633/files#diff-dd2c9d6fc577a3da1b7cf0b6b4a0705a1c71eb5d99c372d5bdda99476362f975L97-L102)
- `vendor`: this directory doesn't actually exist in the `wrangler`
   package directory, there is a root `vendor` directory, but files
   from that are copied to `wrangler-dist` during build

Closes #3013
petebacondarwin pushed a commit that referenced this issue May 17, 2023
Prevents the following being included in the npm tarball:

- `src`: we may want to ship this if we shipped source maps, so
  users could click on internal lines in stack traces, but we don't
  ship source maps to npm, so these aren't needed.
- `miniflare-config-stubs`: these aren't used as of #633
  (https://github.com/cloudflare/workers-sdk/pull/633/files#diff-dd2c9d6fc577a3da1b7cf0b6b4a0705a1c71eb5d99c372d5bdda99476362f975L97-L102)
- `vendor`: this directory doesn't actually exist in the `wrangler`
   package directory, there is a root `vendor` directory, but files
   from that are copied to `wrangler-dist` during build

Closes #3013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Change that will result in breaking existing behavior enhancement New feature or request maintenance Maintenance task quick win Potentially easy/straightforward issue to tackle
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants