Skip to content
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

Merged
merged 4 commits into from Oct 21, 2019
Merged

Update Webpack + deps, remove now unnecessary polyfills #2410

merged 4 commits into from Oct 21, 2019

Conversation

avindra
Copy link
Contributor

@avindra avindra commented Sep 14, 2019

Ready for review. General maintenance work:

  • handles webpack 1 -> 4 migration
    • under the hood, minification has has been replaced with terser (from uglify.js)
  • remove custom utils for Array.isArray and String.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

  • remove es6 promise polyfill from manual test html files

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:

$ grep webpack package.json 
46:    "grunt-webpack": "^3.1.3",
60:    "karma-webpack": "^4.0.2",
67:    "webpack": "^4.40.2"

"typescript": "^2.8.1",
"url-search-params": "^0.10.0",
"webpack": "^1.13.1",
"webpack-dev-server": "^1.14.1"
Copy link
Contributor Author

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.

@avindra
Copy link
Contributor Author

avindra commented Sep 14, 2019

Not sure why Travis isn't building. will check in again when there is a result

@avindra avindra changed the title [wip] update deps Update Webpack 1 -> 4, also general dependency updates Sep 14, 2019
@avindra avindra changed the title Update Webpack 1 -> 4, also general dependency updates Update Webpack 1 -> 4, 2x polyfill removal, other dep updates Sep 14, 2019
Copy link
Contributor Author

@avindra avindra left a 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({
Copy link
Contributor Author

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`,
Copy link
Contributor Author

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() {
Copy link
Contributor Author

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

@avindra avindra changed the title Update Webpack 1 -> 4, 2x polyfill removal, other dep updates Update Webpack + deps, remove now unnecessary polyfills Sep 16, 2019
@avindra
Copy link
Contributor Author

avindra commented Sep 16, 2019

Rebased 7 -> 4 commits using latest master

@avindra
Copy link
Contributor Author

avindra commented Oct 17, 2019

🦗 can I get a review in here?

function isArray(val) {
return toString.call(val) === '[object Array]';
}
var _toString = Object.prototype.toString;
Copy link
Contributor Author

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.
@avindra
Copy link
Contributor Author

avindra commented Oct 21, 2019

pinging @felipewmartins can you review?

@felipewmartins felipewmartins merged commit 189b34c into axios:master Oct 21, 2019
@felipewmartins
Copy link
Collaborator

Thanks @avindra

@felipewmartins
Copy link
Collaborator

@avindra There are failing tests. Are you can see?

felipewmartins added a commit that referenced this pull request Oct 21, 2019
felipewmartins added a commit that referenced this pull request Oct 21, 2019
felipewmartins added a commit that referenced this pull request Oct 25, 2019
* Revert "Update Webpack + deps, remove now unnecessary polyfills (#2410)"

This reverts commit 189b34c.

* Fix build (#2496)

* Change syntax to see if build passes

* Test commit

* Test with node 10

* Test adding all browsers in travis

* remove other browsers when running on travis
@avindra
Copy link
Contributor Author

avindra commented Oct 28, 2019

@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 Promise for IE11 so the tests can run. I'll open a new PR then.

@axios axios locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants