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

Nits #120

Merged
merged 4 commits into from
Nov 10, 2023
Merged

Nits #120

merged 4 commits into from
Nov 10, 2023

Conversation

iVanlIsh
Copy link
Collaborator

@iVanlIsh iVanlIsh commented Nov 9, 2023

No description provided.

index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated
Comment on lines 987 to 988
<dd>Set |request|'s [=target IP address space=] to [=IP address
space/public=]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should throw a TypeError here if we already know the value isn't valid.

I'm still not sure that we really need public in the IPAddressSpace type since we don't have a valid case to use it. I guess that's why I called it RequestTargetAddressSpace in the first place so that we only need to include the valid values that should be used.

Suggested change
<dd>Set |request|'s [=target IP address space=] to [=IP address
space/public=]
<dd>[=Throw=] a {{TypeError}}.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we need this because we need to define if a request's IP address space is less public then the initiator's. In this case, we need to define initiator's IP address space first, which can be public.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is actually that we shouldn't put this general definition in the Fetch API section. I think it will make more sense to put it somewhere more generic.

Copy link
Collaborator

@johnathan79717 johnathan79717 Nov 9, 2023

Choose a reason for hiding this comment

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

We have already defined the public address space in https://wicg.github.io/private-network-access/#ip-address-space-public. I meant we didn't need it in the IDL, i.e. the JS API. My understanding is that we should define what we will use in the Javascript in the IDL.

Copy link
Collaborator

@johnathan79717 johnathan79717 Nov 9, 2023

Choose a reason for hiding this comment

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

If you insist keeping "public" in the IDL, I still think throwing a TypeError here is better than a network error later in the algorithm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I meant we should share the same enum other than create a new one for fetch API only.

I don't think this is a TypeError on one hand, and on the other, "public" is forbidden to bypass mixed content check sounds like a better logic than a fetch request cannot be "public" targetAddressSpace.

index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated
Comment on lines 987 to 988
<dd>Set |request|'s [=target IP address space=] to [=IP address
space/public=]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is actually that we shouldn't put this general definition in the Fetch API section. I think it will make more sense to put it somewhere more generic.

index.src.html Show resolved Hide resolved
Copy link
Collaborator

@johnathan79717 johnathan79717 left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes.

index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
@iVanlIsh iVanlIsh merged commit 203a26b into WICG:main Nov 10, 2023
2 checks passed
github-actions bot added a commit that referenced this pull request Nov 10, 2023
SHA: 203a26b
Reason: push, by iVanlIsh

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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