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

Throw TypeError if passed an offset not supported on the underlying OS #84

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

a-sully
Copy link
Collaborator

@a-sully a-sully commented Jan 6, 2023

Fixes #31


Preview | Diff

@a-sully
Copy link
Collaborator Author

a-sully commented Jan 6, 2023

Please let me know if you'd prefer this to be non-normative text, or if we should add that in addition to the algorithm changes

@a-sully a-sully requested a review from annevk January 6, 2023 23:08
@annevk
Copy link
Member

annevk commented Jan 9, 2023

I see that we also have "throw an {{InvalidStateError}}" which presumably needs to be:

[=throw=] an "{{InvalidStateError}}" {{DOMException}}

Given that, should this be a JS TypeError or should this be of the DOMException variety as well?

And yeah, normally the non-normative description does call out that the algorithm can throw exceptions and which kinds.

@a-sully
Copy link
Collaborator Author

a-sully commented Jan 11, 2023

I see that we also have "throw an {{InvalidStateError}}" which presumably needs to be:

[=throw=] an "{{InvalidStateError}}" {{DOMException}}

Huh, looks like I missed some in #70? Must've been lost while resolving merge conflicts or something...

Given that, should this be a JS TypeError or should this be of the DOMException variety as well?

Prior discussion (#31 (comment)) suggested using TypeError, though I'm happy to make this a DomException if we've changed our mind now that we're seeing this in context with the rest of the algorithm

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I don't have a strong opinion on what exception to use so I'll defer to you on that. Looks good modulo the nit.

Co-authored-by: Anne van Kesteren <[email protected]>
@a-sully a-sully merged commit 149f02e into whatwg:main Jan 11, 2023
@a-sully a-sully deleted the fix-31 branch January 11, 2023 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Seek target range
2 participants