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

Replace url module with lighter self-contained parser #26

Closed
wants to merge 1 commit into from
Closed

Replace url module with lighter self-contained parser #26

wants to merge 1 commit into from

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Jun 21, 2017

Makes the package lighter in both node and the browser.

80KB -> 36KB (saves 55%)

80KB -> 13KB (saves 83% when combined with #25)

I was initially going to keep this as a fork, considering that using url might be preferred in node, but seeing that there are 1127 passing tests I thought of making a PR instead.

@zkat
Copy link
Contributor

zkat commented Mar 7, 2018

I'm going to reject this commit: compatibility with Node itself is higher priority to us than reducing bundle sizes for our dependencies.

I think if your goal is size reduction, finding lighterweight shims for built-in Node.js modules is a better alternative in this specific case. I worry that maintaining our own Regexp for parsing these URLs is going to cause subtle incompatibilities.

Thanks for the contribution, though! #25 was accepted because it's a super sensible patch that we should've been doing anyway. Cheers!

@zkat zkat closed this Mar 7, 2018
@fregante
Copy link
Contributor Author

fregante commented Mar 8, 2018

I feared that would be the case. Thanks for the consideration though 🙂

@fregante fregante deleted the no-url branch September 19, 2018 05:56
fregante added a commit to npmhub/npmhub that referenced this pull request Jan 31, 2021
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.

2 participants