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.

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

Spurious fail caused by fetch("https://some.url.that/only/accepts/post", { method: "POST" }) #188

Closed
jameshfisher opened this issue Nov 13, 2020 · 2 comments
Closed

Spurious fail caused by fetch("https://some.url.that/only/accepts/post", { method: "POST" }) #188

jameshfisher opened this issue Nov 13, 2020 · 2 comments

Comments

@jameshfisher
Copy link

Steps to reproduce

<!doctype html>
<html>
  <script>
    fetch("https://some.url.that/only/accepts/post", { method: "POST" });
  </script>
</html>

Expected behavior

All tests pass, because no requests made by this page will result in a 404.

Actual behavior

not ok 2 load https://some.url.that/only/accepts/post
  ---
    operator: load
    expected: "200 https://some.url.that/only/accepts/post"
    actual:   "getaddrinfo ENOTFOUND some.url.that"
    at: _site/test_case.html:4:12 (inlined JavaScript)

It seems that when hyperlink finds fetch("somestringliteral", { ... }), it will try to GET somestringliteral, and fail if it gets a 404.

Workaround

I can currently outsmart hyperlink by writing

<!doctype html>
<html>
  <script>
    const theUrl = "https://some.url.that/only/accepts/post";
    fetch(theUrl, { method: "POST" });
  </script>
</html>

But this is undesirable, because I have to obfuscate my code to get around the testing tool.

Desired change

  • Option 1: hyperlink should only follow a fetch() URL if it knows for sure that it's a GET request (although I think this is going down a deep rabbithole)
  • Option 2: some kind of escape hatch like Can I mark a file/node to be ignored? #187
@Munter
Copy link
Owner

Munter commented Nov 13, 2020

Fetch had always been difficult to reason about for assetgraph. There's are cases where it's used to fetch internal static assets, which would make sense to check, and in assetgraphs other user cases to include in the build. But this case is obviously wrong behavior, and hyperlink also shouldn't attempt other http methods either.

I'm inclined to say that hyperlink should maybe just assume fetch is not used for static content by default, and not check the link at all.

It maybe it should only follow fetch requests if the URL had a known static file extension.

@papandreou what do you think?

@papandreou
Copy link
Collaborator

I think we should stop modelling fetch And let users opt in with fetch(‘/foo.json’.toString(‘url’)).

Munter added a commit that referenced this issue Nov 13, 2020
@Munter
Avoid ever following JavascriptFetch relations. Fixes #188
7abac61
@Munter Munter mentioned this issue Nov 13, 2020
Merged
@Munter Munter closed this as completed in 672c6b1 Jul 27, 2021
Munter added a commit that referenced this issue Jul 27, 2021
@Munter
Merge pull request #189 from Munter/disable-following-fetch
4c2c6c5 
Avoid ever following JavascriptFetch relations. Fixes #188
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jameshfisher @Munter @papandreou