Skip to content

Conversation

@krisbitney
Copy link
Contributor

@krisbitney krisbitney commented Mar 2, 2023

NOTE - migrating this PR here: polywrap/javascript-client#6

This PR adds URI authority inference to the Uri class, allowing users to import wrappers using common URI formats.

Example:

    let uri = new Uri("https://domain.com/path/to/thing");
    expect(uri.uri).toEqual("wrap://https/https://domain.com/path/to/thing");
    expect(uri.authority).toEqual("https");
    expect(uri.path).toEqual("https://domain.com/path/to/thing");

@krisbitney krisbitney requested a review from dOrgJelli March 7, 2023 16:50
});
});

it("Infers common URI authorities", () => {
Copy link
Contributor

@nerfZael nerfZael Apr 17, 2023

Choose a reason for hiding this comment

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

Common URI authorities seem awkward.
How do we decide which authorities are/should be common and which shouldn't?
Can/should different core libraries (js, rust, python) support different common authorities?
If we're adding ENS, should we add name services of other chains?

This feature would also make it impossible to differentiate between WRAP URIs and other protocol's URIs. (Both would be valid URIs for Uri.parseUri)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "common URI authorities" thing is just the name of the test. It's not part of the feature. I think it's important to test that the feature works with the URI authorities we are commonly using right now. Are there other test cases you have in mind? Would you like more test cases in general?

Regarding the differentiation between WRAP URIs and other protocol URIs with Uri.parseUri, I don't think much has changed. Since we don't require the wrap:// schema prefix, users can already write something like myAuthority/anyPath.tiktok and it would be parsed without issue. This feature only changes the accepted URI format such that if an authority is not found but a schema (e.g. http://) is present, we assume the schema is the authority of a WRAP URI. It's surface-level formatting flexibility. It doesn't change how URIs interact with the toolchain, or which URIs can resolve to wrappers.

…ity-inference

# Conflicts:
#	packages/js/core/src/utils/index.ts
Copy link
Contributor

@cbrzn cbrzn left a comment

Choose a reason for hiding this comment

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

i have a few thoughts:

1- I think the main change here is that we're allowing the user to use authority with the scheme format - While I think it looks good and it might be easier to read (at least from my perspective), this is a change in the URI standard, and this probably introduces more freedom when it comes to defining a URI, which makes me have the question: do we want to give users that freedom?

2- The only URI that can be inferred is HTTP, right? Because of the format it has, but with the other "common" (or used/supported) URIs, this is not the case (if I understand things correctly). So this makes me think that probably this change should be done in the HTTP URI resolver itself (which, if I understand correctly it already has been done) rather than changing the URI standard?

@krisbitney
Copy link
Contributor Author

krisbitney commented Apr 18, 2023

i have a few thoughts:

1- I think the main change here is that we're allowing the user to use authority with the scheme format - While I think it looks good and it might be easier to read (at least from my perspective), this is a change in the URI standard, and this probably introduces more freedom when it comes to defining a URI, which makes me have the question: do we want to give users that freedom?

2- The only URI that can be inferred is HTTP, right? Because of the format it has, but with the other "common" (or used/supported) URIs, this is not the case (if I understand things correctly). So this makes me think that probably this change should be done in the HTTP URI resolver itself (which, if I understand correctly it already has been done) rather than changing the URI standard?

I don't see it as a change to the URI standard, but it seems at least two people do. 🤷 The intention wasn't to change the standard, but rather to provide new syntax sugar. The URI will always be immediately converted to a WRAP URI.

When it comes to UX, we should optimize for the user. I assume we all agree on that. But I realize there could be drawbacks to the change. What are the substantive costs or drawbacks of this change?

It actually does work with other types of URIs, not just HTTP. For example, a common format for an IPFS URI is ipfs://Qm.... The change would support that format.

@namesty
Copy link
Collaborator

namesty commented Apr 18, 2023

I do see the benefit in adding this sugar, as I too find it akward to write things like wrap:://http/http://something.com instead of simply the HTTP Uri. I too agree with this being a change at the parsing level, not really at the standard level (if I am understanding this correctly)

One concern I have is that it could make it more difficult to track the origin of a URI. Can't really think of a scenario where we would specifically need that, but just wanted to throw it out there. Another thing is that it could lead to ambiguity: If multiple authorities could be inferred for a given URI, it would be unclear which authority should be used; but I can't think of a case for that either.

Overall, I think (I'm open to being changed) that maybe the benefits outweigh the cons here. Yesterday I was a bit reluctant, but after thinking it through and seeing the thread, I believe this is probably a good idea.

@dOrgJelli
Copy link
Contributor

Migrating this PR here: polywrap/javascript-client#6

@dOrgJelli dOrgJelli closed this Apr 19, 2023
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.

6 participants