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

request.cache throws "TypeError: This property is not implemented." #10286

Closed
jsejcksn opened this issue Apr 21, 2021 · 11 comments · Fixed by #10784 or #12063
Closed

request.cache throws "TypeError: This property is not implemented." #10286

jsejcksn opened this issue Apr 21, 2021 · 11 comments · Fixed by #10784 or #12063
Assignees
Labels
bug Something isn't working correctly ext/fetch related to the ext/fetch web related to Web APIs

Comments

@jsejcksn
Copy link
Contributor

jsejcksn commented Apr 21, 2021

Edit: It appears this is for all requests:

const request = new Request('https://api.github.com/repos/denoland/deno');
console.log(request.cache); // throws TypeError

In Deno v1.9.1: for each Deno.RequestEvent yielded by Deno.serveHttp, accessing the value of requestEvent.request.cache throws a TypeError.

Here is the source:

get cache() {
webidl.assertBranded(this, Request);
throw new TypeError("This property is not implemented.");
}

This did not occur in 1.9.0 (the value was undefined). See the reproduction case below:

Reproduce:

example.ts:

async function handleConnection(conn: Deno.Conn) {
  for await (const { request, respondWith } of Deno.serveHttp(conn)) {
    try {
      const { cache } = request;
      respondWith(new Response(`Cache mode is: ${cache}`));
    } catch (ex) {
      await respondWith(new Response(ex.toString(), { status: 500 }));
      throw ex;
    }
  }
}

async function main() {
  const hostname = "localhost";
  const port = 8000;
  const url = new URL(`http://${hostname}:${port}/`);

  await Deno.permissions.request({ name: "net", host: url.host });

  const listener = Deno.listen({ hostname, port });
  console.log(`Listening at ${url.href}\nUse ctrl+c to stop`);

  for await (const conn of listener) handleConnection(conn);
}

if (import.meta.main) main();
  1. Start server:
% deno --unstable run example.ts
⚠️  ️Deno requests network access to "localhost:8000". Grant? [g/d (g = grant, d = deny)] g
Listening at http://localhost:8000/
Use ctrl+c to stop
  1. Navigate to http://localhost:8000/ in a browser. Switch back to terminal.
  2. An exception was thrown. stderr:
error: Uncaught (in promise) TypeError: This property is not implemented.
      const { cache } = request;
              ^
    at Request.get cache (deno:op_crates/fetch/23_request.js:343:13)
    at handleConnection (file:///Users/jesse/example/example.ts:4:15)

Version

% deno --version
deno 1.9.1 (release, x86_64-apple-darwin)
v8 9.1.269.5
typescript 4.2.2
@lucacasonato
Copy link
Member

This is working as intended. We do not implement HTTP cache yet, so accessing this field throws. This is consistent with other server side fetch implementations, namely Cloudflare Workers and node-fetch.

@lucacasonato lucacasonato added the working as designed this is working as intended label Apr 21, 2021
@jsejcksn jsejcksn changed the title Accessing request.cache yielded by Deno.serveHttp throws "TypeError: This property is not implemented." request.cache throws "TypeError: This property is not implemented." Apr 21, 2021
@jsejcksn
Copy link
Contributor Author

We do not implement HTTP cache yet, so accessing this field throws. This is consistent with other server side fetch implementations, namely Cloudflare Workers and node-fetch.

@lucacasonato I understand that certain aspects are not implemented, but the behavior of throwing on access is not consistent with other server side implementations. To use your cited example: node-fetch returns undefined when invoking the getter for unimplemented props like cache. Try it:

example.mjs:

import {Request} from 'node-fetch';
const request = new Request('https://api.github.com/repos/denoland/deno');
console.log(request.cache); //=> undefined

@lucacasonato
Copy link
Member

Cloudflare Workers:
image

Try yourself at https://cloudflareworkers.com/.

@lucacasonato
Copy link
Member

lucacasonato commented Apr 21, 2021

Do you have a usecase for why you don't want Request#cache to throw? I think it is better to explicitly state it is not implemented, than returning a value that would never be returned in the browser (like undefined in node-fetch).

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Apr 21, 2021

I think it is better to explicitly state it is not implemented, than returning a value that would never be returned in the browser

It is exactly for this reason that I expect accessing these props to not throw.

It would never throw in a browser and deviating from that requires a divergence in application logic for Deno vs web. In web, we just compare the value of the props, but in Deno 1.9.1 we have to try/catch.

@lucacasonato
Copy link
Member

Again, do you have a use case?

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Apr 21, 2021

Yes. I want to write portable logic that has as little Deno special runtime code as possible. Isn't that one of the major goals of the project?

  • Be browser-compatible.
    • The subset of Deno programs which are written completely in JavaScript and do not use the global Deno namespace (or feature test for it), ought to also be able to be run in a modern web browser without change.

https://deno.land/manual#goals

Emphasis on "without change". Throwing when accessing these properties is not browser-compatible.

@lucacasonato
Copy link
Member

Ok I have thought about this some more, and I guess this does make sense. We should remove all of the unimplemented methods.

@lucacasonato lucacasonato reopened this May 29, 2021
@lucacasonato lucacasonato added bug Something isn't working correctly ext/fetch related to the ext/fetch web related to Web APIs and removed working as designed this is working as intended labels May 29, 2021
@lucacasonato lucacasonato self-assigned this May 29, 2021
@jsejcksn
Copy link
Contributor Author

@lucacasonato What made you backtrack?

@nayeemrmn
Copy link
Collaborator

Throwing when accessing these properties is not browser-compatible.

@jsejcksn That's an incorrect understanding of the compatibility rule. Making it throw marks it as an invalid Deno program so by the rule it doesn't have to work in browsers. Returning a non-spec value can technically violate the rule. BUT returning undefined is only incompatible in pathological cases and is effectively how unimplemented specs are handled. So it may practically be the better choice.

To explain for example why we make the location global without --location throw: it's always defined in browsers, and people having to null-check it for Deno is not how we want to support web APIs. But we don't want people to try-catch around it either, that would destroy the point. We specifically want them to see the error message and fix it by supplying a --location.

Half of that would seem to apply here, we don't want people to have to null-check Request::cache for Deno since it's always defined in browsers. However, unlike with location there isn't a fix other than try-catching around it which is even worse. So leaving it undefined is better.

@lucacasonato
Copy link
Member

What made you backtrack?

That this is how unimplemented functionally in browsers behave. It just isn't there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly ext/fetch related to the ext/fetch web related to Web APIs
Projects
None yet
3 participants