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

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

Closed
axetroy opened this issue Feb 29, 2020 · 18 comments
Closed

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

axetroy opened this issue Feb 29, 2020 · 18 comments
Labels
bug Something isn't working correctly

Comments

@axetroy
Copy link
Contributor

axetroy commented Feb 29, 2020

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
@ry ry added the bug Something isn't working correctly label Feb 29, 2020
@KSXGitHub
Copy link
Contributor

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
Copy link
Contributor

kitsonk commented Mar 3, 2020

@KSXGitHub I believe Pika requires a Deno user agent.

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

@KSXGitHub
Copy link
Contributor

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

@kitsonk
Copy link
Contributor

kitsonk commented Mar 3, 2020

@KSXGitHub it is set here:

deno/cli/http_util.rs

Lines 32 to 35 in eafd40f

headers.insert(
USER_AGENT,
format!("Deno/{}", version::DENO).parse().unwrap(),
);

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
Copy link

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";
  • Confirmed this works as expected in v0.33.0
  • Confirmed this suffers from that URL resolution bug in v0.34.0
  • Confirmed that this runs, but type declarations are ignored in v0.35.0

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
Copy link
Contributor

kitsonk commented Mar 3, 2020

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
Copy link
Contributor

kitsonk commented Mar 4, 2020

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:

  • Pika CDN (and anyone else) keep a chain alive and generates an X-TypeScript-Types for the wrapper modules, that re-export the fixed bundle URL. Deno should then go and fetch the fixed bundle which will have the proper X-TypeScript-Types and everything should work.
  • 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.

@bartlomieju
Copy link
Member

bartlomieju commented Mar 4, 2020

  • 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
Copy link
Contributor

kitsonk commented Mar 4, 2020

@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
Copy link

@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
Copy link
Contributor

kitsonk commented Mar 4, 2020

@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
Copy link

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
Copy link
Contributor

kitsonk commented Mar 6, 2020

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
Copy link

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

kitsonk added a commit to kitsonk/deno that referenced this issue Mar 16, 2020
Refs denoland#4184
Refs denoland#4040

In denoland#4040 we changed the way JavaScript dependencies are analysed in the
TypeScript compiler.  When we encounter a JavaScript file that doesn't
have any types, and `checkJs` is disabled, we stop the analysis.  This
caused situations where when there was a typed file, loading an
untyped file, loading a typed file, we stop the analysis.  We didn't
specifically check this "chaining" behaviour in our tests, and there
were situations in the while where this behaviour was a regression, so
this introduces test to ensure the behaviour as designed is preserved.
@kitsonk
Copy link
Contributor

kitsonk commented Mar 16, 2020

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
Copy link
Contributor Author

axetroy commented Mar 16, 2020

@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
Copy link
Contributor

kitsonk commented Mar 16, 2020

@axetroy there was nothing to fix in Deno. 0.35 should be working again against Pika.dev/cdn. See Fred's comment here: #4184 (comment)

kitsonk added a commit to kitsonk/deno that referenced this issue Mar 17, 2020
Refs denoland#4184
Refs denoland#4040

In denoland#4040 we changed the way JavaScript dependencies are analysed in the
TypeScript compiler.  When we encounter a JavaScript file that doesn't
have any types, and `checkJs` is disabled, we stop the analysis.  This
caused situations where when there was a typed file, loading an
untyped file, loading a typed file, we stop the analysis.  We didn't
specifically check this "chaining" behaviour in our tests, and there
were situations in the while where this behaviour was a regression, so
this introduces test to ensure the behaviour as designed is preserved.
@FredKSchott
Copy link

Thanks @kitsonk!

kitsonk added a commit to kitsonk/deno that referenced this issue Mar 25, 2020
Refs denoland#4184
Refs denoland#4040

In denoland#4040 we changed the way JavaScript dependencies are analysed in the
TypeScript compiler.  When we encounter a JavaScript file that doesn't
have any types, and `checkJs` is disabled, we stop the analysis.  This
caused situations where when there was a typed file, loading an
untyped file, loading a typed file, we stop the analysis.  We didn't
specifically check this "chaining" behaviour in our tests, and there
were situations in the while where this behaviour was a regression, so
this introduces test to ensure the behaviour as designed is preserved.
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
Projects
None yet
Development

No branches or pull requests

7 participants