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

fix: expose url in channel interface #214

Closed
wants to merge 3 commits into from
Closed

Conversation

hugomrdias
Copy link
Contributor

Its useful to be able to get the url from the channel directly, in situations where the connection is passed into a function or class and we want to use the connection url has the default for any other communications like WS.

Instead of input url + connection we can just ask the user for connection.

example : storacha-network/w3up#404

@travis
Copy link
Contributor

travis commented Feb 9, 2023

reopening, I'm going to take over getting storacha-network/w3up#404 merged - @Gozala would love a review from you when you get a chance!

@travis travis reopened this Feb 9, 2023
@travis travis requested a review from olizilla February 14, 2023 08:31
channels won't always have URLs, I think, so make them optional
@alanshaw
Copy link
Member

The HTTP channel already has a url property: https://github.com/web3-storage/ucanto/blob/main/packages/transport/src/http.js#L47

I think you just need an instanceof check:

import { Channel as HttpChannel } from '@ucanto/transport/http'

const { channel } = connection
if (!(channel instanceof HttpChannel)) throw new Error('HTTP channel is required')
const { url } = channel
// do something with URL

I assume the idea is that not all channels will be HTTP in the future...

If you add the type (this PR) then you're still going to need the conditional to check for existence of the url property so you may as well just check with instanceof or duck type it.

travis added a commit to storacha-network/w3up that referenced this pull request Feb 21, 2023
This fixes #344 and makes it possible to connect to staging services with w3ui.

This PR obsoletes #404 per @alanshaw's suggestion in #404 (comment) based on reasoning in web3-storage/ucanto#214 (comment) which points out that some sort of conditional or casting is probably unavoidable here.

My solution casts the channel to have an optional `url` field and looks in that field for a URL before falling back to the default `HOST`.
@travis
Copy link
Contributor

travis commented Feb 21, 2023

closing in favor of a solution to the underlying issue proposed in storacha-network/w3up#440

@travis travis closed this Feb 21, 2023
travis added a commit to storacha-network/w3up that referenced this pull request Feb 21, 2023
This fixes #344 and
makes it possible to connect to staging services with w3ui.

This PR obsoletes #404
per @alanshaw's suggestion in
#404 (comment)
based on reasoning in
web3-storage/ucanto#214 (comment)
which points out that some sort of conditional or casting is probably
unavoidable here.

My solution casts the channel to have an optional `url` field and looks
in that field for a URL before falling back to the default `HOST`.

Have tested this locally with w3ui and verified it makes it possible to
connect to staging services from w3ui - huzzah!
gobengo pushed a commit to storacha-network/w3up that referenced this pull request Apr 11, 2023
This fixes #344 and
makes it possible to connect to staging services with w3ui.

This PR obsoletes #404
per @alanshaw's suggestion in
#404 (comment)
based on reasoning in
web3-storage/ucanto#214 (comment)
which points out that some sort of conditional or casting is probably
unavoidable here.

My solution casts the channel to have an optional `url` field and looks
in that field for a URL before falling back to the default `HOST`.

Have tested this locally with w3ui and verified it makes it possible to
connect to staging services from w3ui - huzzah!
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

4 participants