This repository has been archived by the owner on Feb 26, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
perf: reduce size of published ganache bundle #798
perf: reduce size of published ganache bundle #798
Changes from 4 commits
8b36b6b
ea6613a
1ed4f0a
ce66c28
b36bb00
477d62d
1ac8e79
3bba17e
b0fe10e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 thenode
andcli
bundles?basically:
There was a problem hiding this comment.
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. :-)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
frompackages/core
over topackages/cli
; that way we won't need to expose it at all!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is possible as
core
uses it insrc/packages/core/src/options/index.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi this is how it is currently in
develop
There was a problem hiding this comment.
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