Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

HTTP client factory: Invalid base URL #3332

Closed
acostalima opened this issue Oct 19, 2020 · 2 comments · Fixed by hugomrdias/iso-url#133
Closed

HTTP client factory: Invalid base URL #3332

acostalima opened this issue Oct 19, 2020 · 2 comments · Fixed by hugomrdias/iso-url#133
Labels
need/triage Needs initial labeling and prioritization

Comments

@acostalima
Copy link

acostalima commented Oct 19, 2020

  • Version:

js-ipfs version: 0.49.1-84cfa553ffc717d5d8bf94fdf6a306f182c9aee4
interface-ipfs-core version: ^0.139.1
ipfs-http-client version: ^46.0.1
Repo version: 8
System version: x64/darwin
Node.js version: v14.7.0
Commit: 84cfa55

  • Platform:

MacOS Catalina 10.15.6

Darwin acostalima 19.6.0 Darwin Kernel Version 19.6.0: Thu Jun 18 20:49:00 PDT 2020; root:xnu-6153.141.1~1/RELEASE_X86_64 x86_64

React Native v0.63.2 (tracking RN support at #2813)

  • Subsystem:

HTTP client

Severity:

High (?)

Description:

Call the HTTP client factory with a valid URL to create a new client. The factory throws with TypeError (Invalid base URL).

Steps to reproduce the error:

Issue #3331 has to be fixed first in order to reproduce this one.

import createHttpClient from 'ipfs-http-client';

createHttpClient({ url: 'http://localhost:5002' }); // throws TypeError 'Invalid base URL'

I've traced the issue down to:

const url = new URL(options.url)

and the root cause lies in https://github.com/hugomrdias/iso-url/blob/master/src/url-browser.js#L3 (CC @hugomrdias).
The default value for base cannot be resolved to ''. In fact, as per spec, if left unspecified it should be undefined. The statement new URL('http://localhost:5002', '') throws in Chrome, Firefox and Node.

I'll gladly open a PR to fix this.

@acostalima acostalima added the need/triage Needs initial labeling and prioritization label Oct 19, 2020
@hugomrdias
Copy link
Member

Did this change? i thought we discussed this and thr impl was according to spec. Please PR and ping i will review/merge asap.

@acostalima
Copy link
Author

acostalima commented Oct 19, 2020

@hugomrdias I thought as much too! I had a few items noted down to review and I came across this one again. I did read the docs in MDN again and it's stated this much. I don't remember exactly what led us to conclude everything was alright at the time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
need/triage Needs initial labeling and prioritization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants