-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
Optimise bundling to reduce extension size #195
Conversation
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 understand this PR.
The images are pulled from a CDN and at the same time removed from the repository. I presume that the repository is what ultimately backs the CDN, so I would expect that removing the images from Github won't actually 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.
Solution wise, looking at plugins published by MS, they link to to https://raw.githubusercontent.com. That seems a reasonable way to go. I suppose you would then exclude the images from being bundled with VSCode.
https://marketplace.visualstudio.com/items?itemName=ms-python.python
And see: https://raw.githubusercontent.com/microsoft/vscode-python/main/README.md
SummaryI think in essence it's a question of: "Do we want to store documentation images in git?" - which is probably a point of preference. My answer to that question would have been 'no' as they are not necessarily part of the extension, however looking at Microsoft-developed extensions - storing them within git appears to be their in-house convention - so I have no problem adhering to that. With that in mind, shall I proceed to revert the images to the repository, link their GitHub CDN references on the Detail
Yes, the two distinct approaches to prevent including the images in the bundle appear to be either:
ExamplesExtensions storing images in git
Extensions storing images
|
Okay. Please use tbe raw subdomain. It has two advantages
|
cca9b18
to
e4d5076
Compare
@kieran-ryan I think your force push messed things up a bit. |
e4d5076
to
1061e5f
Compare
1061e5f
to
ecabefb
Compare
@mpkorstanje updated now. Have additionally provided a before and after of the directory structure in the PR description; and changed to a whitelisting strategy for including files in the bundle - which should be less susceptible to introducing redundant artifacts. |
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.
LGTM.
A minor quibble, but fixing that doesn't need a whole review cycle
!CHANGELOG.md | ||
!images/icon.png | ||
!out/*.js | ||
!out/*.wasm |
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.
This seems overly cautious. The entire out folder is expected to go into the plugin.
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.
Agree. Main reason was to continue excluding ‘.map’ files (previous blacklist was ‘**/*.map’).
Perhaps a better strategy would be to whitelist ‘out’ and include an ‘exceptions’ section at the bottom of the ignore file to exclude ‘.map’ files. What do you think?
The discussion on excluding map files in the VSCode Python extension provides more context on reasons for this exclusion. Slowed the extension (at least previously), increased bundle size and only used for debugging to determine exact file and line number that threw an extension - which is not a present concern with this extension given it would apply to two files.
🤔 What's changed?
⚡️ What's your motivation?
Removes need to bundle documentation images - which are not used by the extension itself; and the GitHub URLs are the links used when we visit the extension in the marketplace, so packaging them is redundant.
Reduces extension size from 15,696,312 bytes to 1,038,033 bytes - a 93.4% decrease. Closes #194.
Changing to a bundle whitelisting strategy means not having to update the ignore file every time there are new directories or files included throughout the repository; and is less susceptible to inadvertently introducing unexpected artefacts.
Existing bundle:
Optimised bundle:
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
📋 Checklist: