refactor: remove browserify, switch to webpack #498
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
This is a first step in moving the build from browserify to webpack. The goal is to use webpack for vendor-specific builds as well, replacing current combination of browserify, jq-based json manipulation and inlined shell scripting. This changes: - time required by `yarn build` from ~100s to ~30s - size of window.ipfs content script from 1.1M to 869K - size of entire add-on/dist from 6.5M to 5.6M
|
👀 |
This fixes noisy console logs on CI by switching to safe reported when in CI env
olizilla
reviewed
Jun 11, 2018
webpack.config.js
Outdated
| /* | ||
| occurrenceOrder: true, | ||
| minimizer: [ | ||
| new UglifyJsPlugin({ |
There was a problem hiding this comment.
Any reason why we can't enable uglify here?
There was a problem hiding this comment.
When we run webpack with -p minification via uglify it is enabled with default options.
I've left this section in case we want to customize options that are passed to uglify, but defaults seem to works just fine. Will probably remove it before merge.
webpack.config.js
Outdated
| alias: { | ||
| // These are needed because node-libs-browser depends on outdated | ||
| // versions | ||
| // zlib: can be dropped once https://github.com/devongovett/browserify-zlib/pull/18 is shipped |
There was a problem hiding this comment.
browserify/browserify-zlib#18 is shipped 🚢
webpack.config.js
Outdated
| // versions | ||
| // zlib: can be dropped once https://github.com/devongovett/browserify-zlib/pull/18 is shipped | ||
| zlib: 'browserify-zlib', | ||
| // http: can be dropped once https://github.com/webpack/node-libs-browser/pull/41 is shipped |
There was a problem hiding this comment.
webpack/node-libs-browser#41 is shipped 🚢
- removed custom options that we don't need to use for now - removed workarounds that are no longer needed
- shared webpack config as a base for all targets - separates build targets for background page, GUI and content scripts - bundled webext polyfill for chrome, removed two calls to `tabs.executeScript` - detects and rebuilds ipfs proxy content script on changes (`yarn watch`) - removes browserify libs and other leftovers
This is a PR on top of #498 - `add-on/dist/` size decreased to `5.5M` - shared webpack config as a base for all targets - separates build targets for background page, GUI and content scripts - bundled webext polyfill for chrome, removed two calls to `tabs.executeScript` - detects and rebuilds ipfs proxy content script on changes (`yarn watch`) - removes browserify libs and other leftovers
|
Added changes from #499 to this PR, which brings:
|
This will make logs non-interactive and more detailed
lidel
added a commit
that referenced
this pull request
Jun 15, 2018
This PR enables Firefox users to disable creation of `window.ipfs` attribute via _Preferences_ screen, solving fingerprinting issue raised in #451. It requires webpack, so should be merged after #498 ### Background Existing `tabs.executeScript` API with `runAt: 'document_start'` flag was executing too late and page scripts were unable to detect `window.ipfs` correctly. More info on the underlying issue: #368 #362 (comment) As a workaround that worked in both Chrome and Firefox, we were forced to always load content script via manifest definition. This meant no control over when it is loaded and no easy off switch. If `window.ipfs` was disabled via _Preferences_, it was throwing an Error on access, but remained in the scope. Mozilla added [`contentScripts`](https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contentScripts/register) API in Firefox 59. The key difference between `tabs.executeScript` and `contentScripts` API is that the latter provides guarantee to execute before anything else on the page, passing [our synchronous smoke test](https://ipfs-shipyard.github.io/ipfs-companion/docs/examples/window.ipfs-fallback.html). ### Known Issues Chrome does not provide `contentScripts` API so it will continue to statically load `content_script` via manifest. Hopefully with time, other browser vendors will adopt the new API.
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.
This PR switches the build from browserify to webpack.
I tried to make non-invasive and easy to review, so it does not change project layout.
Immediate gains introduced by this PR:
yarn builddecreased from ~100s to ~30syarn watchproduces human-readable code that is easier to debugLong term goal is to use webpack for vendor-specific builds as well,
replacing current combination of browserify, jq-based json manipulation
and inlined shell scripting with more programmatic approach that
is easier to maintain.
Tasks
This PR is ready for review & merge if all below are checked:
yarn buildandyarn watchwork as expected.jsbigger than 4MB (limit introduced by addon-linter)(we dont want js-ipfs and js-ipfs-api in common bundle, as it slows down UI popups)
yarn watchshould generate human-readable code and/or source mapsyarn release-buildproduces the same output every timeFuture Improvements
These things are not included in this PR:
.jsare not copied to/dist/)