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
Update Webpack + deps, remove now unnecessary polyfills #2410
Conversation
"typescript": "^2.8.1", | ||
"url-search-params": "^0.10.0", | ||
"webpack": "^1.13.1", | ||
"webpack-dev-server": "^1.14.1" |
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.
Only core webpack is used, so just removing the dev server dep.
Not sure why Travis isn't building. will check in again when there is a result |
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.
pinging @felipewmartins . left some markers about why i removed or added certain things
|
||
if (uglify) { | ||
config.plugins.push( | ||
new webpack.optimize.UglifyJsPlugin({ |
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.
mode
is now the canonical way to build with weback.
per the migration guide, the plugins can simply be removed: https://webpack.js.org/migrate/4/#deprecatedremoved-plugins
entry: './index.js', | ||
output: { | ||
path: 'dist/', | ||
path: `${__dirname}/dist`, |
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.
absolute path is required for webpack 4 now.
per the docs, __dirname
seems appropriate
https://nodejs.org/docs/latest/api/modules.html#modules_dirname
@@ -20,7 +20,7 @@ module.exports = function enhanceError(error, config, code, request, response) { | |||
error.response = response; | |||
error.isAxiosError = true; | |||
|
|||
error.toJSON = function() { | |||
error.toJSON = function toJSON() { |
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.
resolved lint warning about anonymous functions being banned by adding a congruent name
Rebased 7 -> 4 commits using latest |
🦗 can I get a review in here? |
function isArray(val) { | ||
return toString.call(val) === '[object Array]'; | ||
} | ||
var _toString = Object.prototype.toString; |
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.
Changed this to start with _
, to avoid having to place an eslint directive.
It also functions as a callout to potentially hacky / legacy code
* handles webpack 1 -> 4 migration
assume `Promise` is available, or polyfilled by the consumer
String.protoype.trim has good coverage (including IE9) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/Trim Also, the http adapter already uses the native method.
in IE9. So lets remove the custom polyfill. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray also resolves a few lint issues
pinging @felipewmartins can you review? |
Thanks @avindra |
@avindra There are failing tests. Are you can see? |
This reverts commit 189b34c.
This reverts commit 189b34c.
@felipewmartins I didnt' see these errors on my PR. Looks like PR builds and master build have different configurations. I'll try to see how we can safely polyfill |
Ready for review. General maintenance work:
Array.isArray
andString.prototype.trim
, which are both covered, even in IE9. Per the README, IE11 is the target supported browser, so we should be ok here.developer changes
Motivation: the main motivation is to get on to a modern version of Webpack to enable use of modern JavaScript syntax (es6+) and multiple export formats (mjs, etc). In the future we may want to test the performance of other bundlers, but the assumptions around webpack are still tight: