Skip to content

Add explicit parsing rules for FETCH integration #87

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
Apr 21, 2022
Merged

Add explicit parsing rules for FETCH integration #87

merged 16 commits into from
Apr 21, 2022

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Feb 27, 2022 •
edited by pr-preview bot
Loading

Depends on whatwg/fetch#1406
Closes #42
Closes #55
Closes #69

Preview | Diff

@noamr noamr mentioned this pull request Feb 27, 2022
Merged
3 tasks
@noamr noamr requested a review from yoavweiss February 27, 2022 14:42
This was referenced Feb 27, 2022
Closed
Closed
Closed
@noamr
Copy link
Contributor Author

noamr commented Feb 28, 2022

💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Feb 28, 2022, 8:36 AM UTC).

More

Build succeeded, not sure why it still says "failed to build"

yoavweiss
yoavweiss approved these changes Feb 28, 2022
View reviewed changes
Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

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

LGTM % nits

It might've been nicer if we could fit some of it into a structured field processing model, but it doesn't seem to fit.

index.html Outdated

1. Let |position| be a [=string/position variable=], initially pointing at the start of |field|.

1. Let |rawName| be the result of [=collecting a sequence of code points=] that are not equal to U+003B (;), given |position|.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rephrase that in terms of condition and input, similar to how the algorithm is defined?
So something like "...be the result of collecting... from field meeting the condition of the codepoints not being equal to ;, given position.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to be consistent with FETCH, everything there uses this style:
e.g. https://fetch.spec.whatwg.org/#simple-range-header-value

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, just add a "from field" then, to clarify which string this is reading from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

index.html Outdated

1. Let |rawName| be the result of [=collecting a sequence of code points=] that are not equal to U+003B (;), given |position|.

1. Let |name| be the result of [=strip leading and trailing ASCII whitespace|stripping=] |rawName|.
Copy link
Contributor

Choose a reason for hiding this comment

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

The algorithm is defined as operating in place rather than returning a result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

index.html Outdated

1. Advance |position| by 1.

1. Let |rawParamName| be the result of [=collecting a sequence of code points=] from |field| that are not equal to U+003D (=), given |position|.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@noamr
Copy link
Contributor Author

noamr commented Feb 28, 2022

LGTM % nits

It might've been nicer if we could fit some of it into a structured field processing model, but it doesn't seem to fit.

It would have been nice, but it doesn't work with a combination of tokens and quoted strings.

@noamr
Copy link
Contributor Author

noamr commented Feb 28, 2022 •
edited
Loading

Will merge once the corresponding FETCH PR is merged, with a ref to the new struct item.

@noamr noamr mentioned this pull request Mar 13, 2022
Closed
20 tasks
@noamr noamr closed this Apr 11, 2022
@noamr noamr reopened this Apr 11, 2022
annevk
annevk reviewed Apr 20, 2022
View reviewed changes
@noamr @annevk
Update index.html
e3cc910 
Co-authored-by: Anne van Kesteren <[email protected]>
annevk pushed a commit to whatwg/fetch that referenced this pull request Apr 21, 2022
@noamr
Add Server-Timing header infrastructure
14898c0 
For w3c/server-timing#87.

(This does not deal with support in trailer fields, that's tracked by w3c/server-timing#68.)
@noamr noamr closed this Apr 21, 2022
@noamr noamr reopened this Apr 21, 2022
@noamr noamr merged commit 4763cb7 into gh-pages Apr 21, 2022
@noamr noamr deleted the rig-parsing branch April 21, 2022 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yoavweiss yoavweiss yoavweiss approved these changes

+1 more reviewer

@annevk annevk annevk left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Creation time of PerformanceServerTiming objects unclear Fetch integration is missing Evaluate current parsing error strategy
3 participants
@noamr @yoavweiss @annevk