Optimize images only during production build #7194
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gudahtt
force-pushed
the
optimize-images-only-during-production-build
branch
from
b654549
to
9a9694a
Compare
Gudahtt
force-pushed
the
optimize-images-only-during-production-build
branch
from
9a9694a
to
d5db5ea
Compare
Builds ready [d5db5ea]
|
danfinlay
previously approved these changes
Sep 19, 2019
|
Well the image optimization step was a recent addition (#7176), and it ran in parallel to other work so the total reduction in time is small. Also it was already excluded from dev builds - just not the test build needed for e2e tests. So don't expect much 😛. It should reduce CI build times slightly though. |
danjm
previously approved these changes
Sep 20, 2019
Image optimization is fairly slow (over a minute), and isn't necessary for test or development builds. It is now only run as part of the `build` gulp task, which is used during `gulp dist`.
There were two high-level tasks and one style formatting task that were not used by any npm scripts, so were probably unused generally. The `dev` task was a duplcate of `dev:extension`. The `build:extension` task was useful for just building the extension without performing other steps required by the final production bundle, but it was broken. It didn't correctly build the ui-libs and bg-libs required. Instead of fixing it, it has been removed until the handling of those separate library bundles is simplified. The style formatting task seems like it could be useful, but I'm unsure about keeping it around as opt-in, as in practice it'll just end up being ignored. Moreover the library authors themselves are recommending switching to `prettier`, so I think we're better off removing it for now, then considering using `prettier` if we want to introduce something like this again. The stylelint dependency was added because it's a peer dependency of gulp-stylelint that should have already been listed among our dependencies. It hadn't caused a problem before because it happened to be a transitive dependency of gulp-stylefmt, which is no longer needed and has been removed.
Gudahtt
force-pushed
the
optimize-images-only-during-production-build
branch
from
d5db5ea
to
0e13aa8
Compare
Builds ready [0e13aa8]
|
danjm
approved these changes
Sep 21, 2019
Gudahtt
added a commit
to Gudahtt/metamask-extension
that referenced
this pull request
Sep 27, 2019
* origin/develop: (56 commits) Add advanced setting to enable editing nonce on confirmation screens (MetaMask#7089) Add migration on 3box imports and remove feature flag (MetaMask#7209) ci - install deps - limit install scripts to whitelist (MetaMask#7208) Add a/b test for full screen transaction confirmations (MetaMask#7162) Update minimum Firefox verison to 56.0 (MetaMask#7213) mesh-testing - submit infura rpc requests to mesh-testing container (MetaMask#7031) obs-store/local-store should upgrade webextension error to real error (MetaMask#7207) sesify-viz - bump dep for visualization enhancement (MetaMask#7175) address book entries by chainId (MetaMask#7205) Optimize images only during production build (MetaMask#7194) Use common test build during CI (MetaMask#7196) Report missing `en` locale messages to Sentry (MetaMask#7197) Verify locales on CI (MetaMask#7199) updated ganache and addons-linter (MetaMask#7204) fixup! add user rejected errors add user rejected errors update json-rpc-engine use eth-json-rpc-errors Remove unused locale messages (MetaMask#7190) Remove unused components (MetaMask#7191) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Image optimization is fairly slow (over a minute), and isn't necessary for test or development builds. It is now only run as part of the
buildgulp task, which is used duringgulp dist.A few unused gulp tasks have been removed as well. There were two high-level tasks and one style formatting task that were not used by any npm scripts, so were probably unused generally.
The
devtask was a duplcate ofdev:extension. Thebuild:extensiontask was useful for just building the extension without performing other steps required by the final production bundle, but it was broken. It didn't correctly build the ui-libs and bg-libs required. Instead of fixing it, it has been removed until the handling of those separate library bundles is simplified.The style formatting task seems like it could be useful, but I'm unsure about keeping it around as opt-in, as in practice it'll just end up being ignored. Moreover the library authors themselves are recommending switching to
prettier, so I think we're better off removing it for now, then considering usingprettierif we want to introduce something like this again.The
stylelintdependency was added because it's a peer dependency ofgulp-stylelintthat should have already been listed among our dependencies. It hadn't caused a problem before because it happened to be a transitive dependency ofgulp-stylefmt, which is no longer needed and has been removed.