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

Replace xhr with fetch #35

Closed
wants to merge 4 commits into from
Closed

Replace xhr with fetch #35

wants to merge 4 commits into from

Conversation

koddsson
Copy link
Contributor

Best viewed, commit and commit.

Mostly wanted to drop the text/html; fragment accept header and did that in 6703c3e. This matches <auto-check> which does not check for text/html; fragment anymore. Otherwise cleaned up a few things so it's more similar to the work we've done in <auto-check>. Using fetch and minor things.

@koddsson koddsson requested a review from a team January 21, 2020 16:51
@muan
Copy link
Contributor

muan commented Jan 21, 2020

Mostly wanted to drop the text/html; fragment accept header and did that in 6703c3e.

Can you add more on the reasoning for this change? <auto-complete> needs a HTML fragment to function, like <remote-input>; <auto-check> does not.

@koddsson
Copy link
Contributor Author

Can you add more on the reasoning for this change?

Apparently nginx doesn't handle spaces in mimetypes so we need to do something here. <auto-check> didn't seem to use it at all so I opted to drop it.

<auto-complete> needs a HTML fragment to function

Could you expand on this? I didn't see any checks that make sure that the content type are text/html; fragment.

Copy link
Contributor

@dgraham dgraham 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 the same question as @muan regarding the removal of content negotiation. We need to be able to identify these requests in the server so full error pages and 404 pages are not served in responses. Removing content negotiation in auto-check needs to be revisited as well.

@koddsson
Copy link
Contributor Author

I have the same question as @muan regarding the removal of content negotiation. We need to be able to identify these requests in the server so full error pages and 404 pages are not served in responses. Removing content negotiation in auto-check needs to be revisited as well.

Yeah, looking at this more I didn't realize that we have this header in so many components already and it seemed to be missing in <auto-check> so I figured it wasn't needed. I'm gonna have to figure out a better solution to this.

I'm still not sure auto complete needs the text/html; fragment content type to function since it doesn't check for it in the code.

@koddsson koddsson closed this Jan 27, 2020
@koddsson koddsson deleted the replace-xhr-with-fetch branch January 27, 2020 13:17
@muan
Copy link
Contributor

muan commented Jan 27, 2020

I'm still not sure auto complete needs the text/html; fragment content type to function since it doesn't check for it in the code.

What I meant is that the element needs a HTML fragment response, and users of the element needs to know that somehow. This is done properly through content negotiation. Content negotiation is thus essential because the element requires this specific type of content to do its primary job; otherwise, apps can be sending back a JSON string because the element did not ask for the right thing, and the element wouldn’t work.

I view “asking for the right thing” as the element’s responsibility, especially considering all the troubles we’ve had with <include-fragment> not asking for the right thing and requiring us to add an accept attribute.

<auto-check> on the other hand does not use the response content, so I’d argue content negotiation isn’t essential.

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.

3 participants