-
Notifications
You must be signed in to change notification settings - Fork 299
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2392 +/- ##
======================================
Coverage 38.2% 38.2%
======================================
Files 300 300
Lines 12518 12518
Branches 1649 1649
======================================
Hits 4782 4782
Misses 7481 7481
Partials 255 255 Continue to review full report at Codecov.
|
Great catch @CrossR ! Thanks for cleaning this up 😄
Wow, that's massive - really glad you caught that! 💯 I noticed the Mac
I like this idea - it's definitely the right strategy - the 'everything but...' strategy is clunky and unreliable (and makes it easy for some of these issues to sneak in!). We shouldn't need the
That's a great idea! I like the idea of having a test that validates that the installer isn't greater than a certain size - it builds up our safety net to keeps issues like this from popping in later, unexpectedly. |
I've added this now, and it looks like just by specifying "node_modules" it has included the correct modules. I've not checked it exactly yet, but at least the modules haven't changed at all. Hasn't changed the size too much (5~MB) on my end, but does help overall, and stops issues coming up again when we add new folders. I think I've included only what is needed, but I'd appreciate if someone else could take a quick look over it/look at your install folder and see if what I've included/excluded looks reasonable. |
@@ -20,7 +20,7 @@ const packageJsonContents = fs.readFileSync(path.join(__dirname, "..", "package. | |||
const packageMeta = JSON.parse(packageJsonContents) | |||
const { version, name } = packageMeta | |||
const prodName = packageMeta.build.productName | |||
const pathVariable = "{app}\\resources\\app\\cli\\windows\\" | |||
const pathVariable = "{app}\\resources\\app\\cli\\win\\" |
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 swapped this to take advantage of the ${os}
variable in electron_builder
, such that we only include the CLI script for the needed platform.
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.
Awesome!
Currently, we are packing some folders that I don't think we need to into the final installers.
This includes
ui-tests
, themain
src folder,.github
folder, but the worst offender is the.oni_build_cache
folder. On my machine with justripgrep
in it, its 2MB, but I think on Mac wherenvim
is also cached, it could be almost 50MB, which is a big chunk of the total size of that folder. I'm basing that on the factnvim
for Windows is about 45MB.Assuming this doesn't break everything, is there anything else we can remove? The
README.md
etc is also included and so forth...is it worth swapping from an "Everything but ..." to specifying the individual folders we need? We currently include the.map
files, which I'm not sure how much value they add.Setting up this PR to test the CI builds, since that cache is only added for them. Its potentially also worth us adding some size constraints? Or maybe warnings if the installer size deviates from some set value?