denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
96.6k stars 5.33k forks source link

didn't respect `x-typescript-types` response header #4184

Closed axetroy closed 4 years ago

axetroy commented 4 years ago

The submodule of https://cdn.pika.dev/fp-ts contains header information, but the declaration file is not downloaded

{
  "headers": {
    "access-control-allow-origin": "*",
    "cache-control": "no-store, no-cache",
    "x-typescript-types": "/-/fp-ts@v2.5.3/dist=es2019/types/index.d.ts",
    "x-ratelimit-remaining": "250",
    "x-now-cache": "MISS",
    "x-ratelimit-limit": "250",
    "x-powered-by": "Express",
    "server": "now",
    "date": "Sat, 29 Feb 2020 12:11:55 GMT",
    "strict-transport-security": "max-age=63072000",
    "x-now-trace": "tpe1",
    "content-type": "application/javascript; charset=utf-8",
    "x-ratelimit-reset": "1582978500",
    "x-now-id": "tpe1:sfo1:z6h2z-1582978315332-d57a553dae6d",
    "etag": "W/\"4bf1b-Qscuf3Ki/CoTL8CDQWlVn5KGfTo\""
  },
  "url": "https://cdn.pika.dev/-/fp-ts@v2.5.3/dist=es2019/fp-ts.js"
}

Source Code

import { array } from "https://cdn.pika.dev/fp-ts";

const M = array.getMonoid<number>();
console.log("concat Array", M.concat([1, 2], [2, 3]));
$ deno -V
deno 0.35.0
$ deno run mod.ts
# or deno fetch mod.ts
KSXGitHub commented 4 years ago

Weird, curl -I command does not return x-typescript-types.

curl -I -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.122 Safari/537.36' https://cdn.pika.dev/fp-ts
HTTP/2 200 
content-type: application/javascript; charset=utf-8
link: </-/fp-ts@v2.5.3-ioq67MmHjAkSIzNDWcun/dist=es2019/fp-ts.js>; rel=modulepreload
date: Tue, 03 Mar 2020 05:40:55 GMT
x-powered-by: Express
access-control-allow-origin: *
x-import-url: /-/fp-ts@v2.5.3-ioq67MmHjAkSIzNDWcun/dist=es2019/fp-ts.js
x-ratelimit-limit: 250
cache-control: private, max-age=300
x-ratelimit-remaining: 247
x-pinned-url: /fp-ts@v2.5.3-ioq67MmHjAkSIzNDWcun
x-ratelimit-reset: 1583214300
etag: W/"4e7-YHg9F3QYp1SPJ3H0z+Y1fUENkK4"
content-length: 0
x-now-cache: MISS
x-now-trace: hkg1
server: now
x-now-id: hkg1:sfo1:dq974-1583214055575-463f01f20edd
strict-transport-security: max-age=63072000
kitsonk commented 4 years ago

@KSXGitHub I believe Pika requires a Deno user agent.

@axetroy I believe it is related to the JS dependency analysis, related to #4197

KSXGitHub commented 4 years ago

@kitsonk What is deno user agent? How do I find it?

kitsonk commented 4 years ago

@KSXGitHub it is set here:

https://github.com/denoland/deno/blob/eafd40feabaf14f9f88748c66fa319519fd69072/cli/http_util.rs#L32-L35

But you are right, even with that, the header is missing:

$ curl -I -H 'User-Agent: Deno/0.35.0' https://cdn.pika.dev/fp-ts@^2.4.3

I thought I had fixed it last week @axetroy in #4120 and I think I did, but whatever is in the cache doesn't seem to be coming from Pika anymore.

Paging @FredKSchott

FredKSchott commented 4 years ago

Hey, y'all! https://cdn.pika.dev/fp-ts@^2.4.3 is the semver lookup URL, which then immediately loads/wraps the /-/ resource URL. The actual package resource URL (in this case: https://cdn.pika.dev/-/fp-ts@v2.5.3/dist=es2019/fp-ts.js) appears to be correctly exporting the header (see example in OP).

import * as ns "https://cdn.pika.dev/fp-ts@^2.4.3";

Has the behavior changed to only fetch types that are resolved off of the user's top-level import and ignore the rest? We can change to support this, but would want to make sure that this is intentional behavior first.

kitsonk commented 4 years ago

Hmmm... ah, ok... I don't think the change in behaviour was intentional. We should support it, that flow (but we currently don't test it). Ok, investigating.

kitsonk commented 4 years ago

Ok, did some thinking about this, and not totally sure what the right answer is.

We had reports of issues with Deno being slow when loading large JavaScript dependencies, and challenges with mis-identifying imports. So we made a change where allowJs is off by default. It means that when Deno tries to load a JavaScript file and the JavaScript file doesn't have a valid .d.ts to substitute (via @deno-types or X-TypeScript-Types), the Deno compiler "stops" it analysis and doesn't dig any deeper.

So what is currently happening in this situation is, a module imports https://cdn.pika.dev/fp-ts which is a JavaScript file (based on its media type), and there is no "substitute" type definition. The Deno compiler sees it as a JavaScript file without types and stops going any further and says that all the imports into the importing module are any type. When Deno starts loading the modules in the runtime, it see the plain JavaScript file importing other JavaScript files with X-TypeScript-Types headers, but it is the Deno compiler that actually loads those (basically, when the compiler asks for a JavaScript module that has a types associated with it, the Deno cache returns the .d.ts file instead of the JavaScript file to the compiler, it effectively lies. Because the compiler is never lied to, the file isn't cached.

So it used to work with allowJs because the Deno compiler would look at the JavaScript wrapper and go off and track the types from actual module and track what the JavaScript was doing and re-export the types along with it.

So given all this, there are two potential fixes, and I am not sure which is best:

bartlomieju commented 4 years ago
  • Deno rolls back the allowJs and loads all JavaScript files into the compiler irrespective of if they have types, to avoid this chain breaking, but the disadvantage is that the compiler wouldn't be able to tell the difference between a small wrapper JavaScript file and a huge untyped JavaScript dependency until it loaded it, parsed it, and wasted everyone's time.

@kitsonk there is #4140 that I was supposed to carry on; and my eyes are still set on the thing we discussed in #4068. Would keeping allowJs and loading/parsing using SWC rather than TS compiler for that speed things up? (To speak more generally I'd like to do as much as TS compiler does currently using SWC, only leave actual type checking to TS compiler)

kitsonk commented 4 years ago

@bartlomieju I don't think you are going to get and AST out of swc that TS can use any time soon, and in this case TS needs to fully parse the JavaScript source in order to figure out what the exports and import type shape is to solve this gap. The first step is to do the dependency analysis in swc and eliminate the preprocess in the compiler, but that wouldn't solve this problem.

FredKSchott commented 4 years ago

@kitsonk if you all need it, we can export the types from the lookup URL as well. I'll need a few days to implement, though.

kitsonk commented 4 years ago

@FredKSchott I don't know if it is easier, or more "future proof" but all that needs to be done is serving up of the semver wrapper module as a TypeScript and passing it in the X-TypeScript-Types header. I believe even if the wrapper type took a query parameter to modify the served media type, it would work, like something like if https://cdn.pika.dev/fp-ts@^2.4.3 simply returned: x-typescript-types: /fp-ts@^2.4.3?ts which served the same file again but with a content-type: application/typescript; charset=utf-8.

FredKSchott commented 4 years ago

okay, the CDN now returns types on lookup URLs as well as resource URLs! I'm seeing expected behavior again using Pika CDN with v0.35.0.

@kitsonk can we add tests for this sort of setup to keep things working in future releases? How do you feel about remote integration tests that test against our CDN directly?

kitsonk commented 4 years ago

How do you feel about remote integration tests that test against our CDN directly?

I'm fine with that, though I would want @ry's opinion. The only thought is that we might want a set of "3rd party integration" tests that won't break the build and are only advisory in the build, versus breaking the build.

I can certainly model what we are talking about here in the integration tests though to catch any regressions. There were whole classes of stuff I broke with trying to get us to handle JavaScript better in the compiler and I apologise.

FredKSchott commented 4 years ago

np! If modeling locally is possible, then that sounds just as good to me

kitsonk commented 4 years ago

So, this issue is effectively an external bug, which has been fixed.

We intentionally changed the behaviour of Deno in #4040, which highlighted that we had coupled to some unintentional behaviour, which Pika.dev/cdn fixed. #4386 adds some tests to make the intentional behaviour of how X-TypeScript-Types works explicit so we won't accidentally break it in the future.

So my recommendation is to close this issue as external fixed.

axetroy commented 4 years ago

@kitsonk Thanks for your great work.

We can keep this issue until the next version been released.

I will verify if it is fixed and then close the issue

kitsonk commented 4 years ago

@axetroy there was nothing to fix in Deno. 0.35 should be working again against Pika.dev/cdn. See Fred's comment here: https://github.com/denoland/deno/issues/4184#issuecomment-595609634

FredKSchott commented 4 years ago

Thanks @kitsonk!