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

Removed import of url module in browser build due to huge size overhead… #4594

Merged
merged 2 commits into from Apr 26, 2022

Conversation

DigitalBrainJS
Copy link
Contributor

@DigitalBrainJS DigitalBrainJS commented Apr 8, 2022

PR #3544 added Malformed URL and protocol checks by importing standard url node.js module which led to a huge size overhead of the client build.
This PR reverts Malformed URL check because:

  • huge size overhead (it was 62 KB, now - 175 KB) due to importing url module, while we're trying to keep Axios lightweight, aren't we?
  • it doesn't make much sense for the client platform, since the URL will be validated, encoded, and fixed by the XMLHTTPRequest object internally.
  • added code is used only for validating, not for Axios logic.

… overhead;

Removed pointless `Malformed URL` checking in  client build;
@@ -78,6 +78,7 @@
"unpkg": "dist/axios.min.js",
"typings": "./index.d.ts",
"dependencies": {
"eslint-g": "^1.3.4",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is use of this package?

https://www.npmjs.com/package/eslint-g

Copy link
Contributor Author

@DigitalBrainJS DigitalBrainJS Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. That was a typo. Apparently, the only purpose of this package is for people to install it by mistake. A very dangerous precedent. It was a typo in a missing space - that's when my IDE required an eslint update. So when you type "npm -i eslint -g" with missing space before -g, you will install this scam. I'm not sure how I missed this change in the commit, but in any case, npm should disallow such names or show an additional prompt to avoid problems like this.

@kumardeepakxyz
Copy link

kumardeepakxyz commented Apr 26, 2022

@DigitalBrainJS So what is the fix for this on the Client-side?

Because webpack build gives the error Module not found: Error: Can't resolve 'url' in '.../node_modules/axios/lib/adapters'

Should we install the url package separately or is there something else that is not documented?

@Jiaocz
Copy link

Jiaocz commented Apr 26, 2022

@DigitalBrainJS So what is the fix for this on the Client-side?

Because webpack build gives the error Module not found: Error: Can't resolve 'url' in '.../node_modules/axios/lib/adapters'

Should we install the url package separately or is there something else that is not documented?

lib/adapters/xhr.js#L10
this removed use of url, after merged and release, the error will solve. you can now install url to temp solve this.
@kumardeepakxyz

@jasonsaayman jasonsaayman merged commit 4f7e3e3 into axios:master Apr 26, 2022
3 checks passed
@kumardeepakxyz
Copy link

kumardeepakxyz commented Apr 26, 2022

@DigitalBrainJS So what is the fix for this on the Client-side?
Because webpack build gives the error Module not found: Error: Can't resolve 'url' in '.../node_modules/axios/lib/adapters'
Should we install the url package separately or is there something else that is not documented?

lib/adapters/xhr.js#L10 this removed use of url, after merged and release, the error will solve. you can now install url to temp solve this. @kumardeepakxyz

Yes, it's solved in 0.27.1 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants