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

Support URLs #34

Merged
merged 1 commit into from
Oct 14, 2016
Merged

Support URLs #34

merged 1 commit into from
Oct 14, 2016

Conversation

hemanth
Copy link
Contributor

@hemanth hemanth commented Oct 14, 2016

WRT to #33

Copy link
Member

@bijection bijection left a comment

Choose a reason for hiding this comment

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

Looks good!

@bijection bijection merged commit 915fadb into naptha:master Oct 14, 2016
@hemanth
Copy link
Contributor Author

hemanth commented Oct 14, 2016

👍

@zzarcon
Copy link
Contributor

zzarcon commented Oct 14, 2016

Probably it's a bit too late but I just saw this, so, are you guys sure about adding those 2 new dependencies? is-url it's just a one line function and isomorphic-fetch in the other hand adds quite some dependencies.

Also we are currently using XMLHttpRequest in a couple of places. So my is point is, I really appreciate the job done in this PR but, do we really need those dependencies? :)

@hemanth
Copy link
Contributor Author

hemanth commented Oct 14, 2016

We can't use XMLHttpRequest in node and using 2 new deps is no harm?

@zzarcon
Copy link
Contributor

zzarcon commented Oct 14, 2016

@hemanth yeah I know, but we could have just used the http module in node and XMLHttpRequest in the browser.

Anyways, it's not a big deal, I just wanted to know what do you think about my concern.

@bijection
Copy link
Member

I think the extra dependencies don't matter too much given the size of the other files involved here, but I've pushed a change switching to node-fetch instead of isomorphic-fetch, as (in node) the latter is a thin wrapper around the former.

@hemanth
Copy link
Contributor Author

hemanth commented Oct 17, 2016

Cool, I was thinking of node-fetch too.

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.

3 participants