FredKSchott / snowpack

ESM-powered frontend build tool. Instant, lightweight, unbundled development. ✌️
https://www.snowpack.dev
MIT License
19.48k stars 922 forks source link

Request for CDN: Convert TypeScript triple-slash directive to x-typescript-types http header #244

Closed KSXGitHub closed 4 years ago

KSXGitHub commented 4 years ago

Some JavaScript files contain the following line:

/// <reference types="./types.d.ts" />

It would be cool if cdn.pika.dev provide TypeScript header file based on this file.

FredKSchott commented 4 years ago

Done! This is ready to go out tomorrow, but can you share a package where you've seen this where you'd like it fixed, so that I can confirm before deploying?

KSXGitHub commented 4 years ago

but can you share a package where you've seen this where you'd like it fixed

FredKSchott commented 4 years ago

Fixed! Thanks for filing!

KSXGitHub commented 4 years ago

@FredKSchott curl -I https://cdn.pika.dev/... in my machine no longer return x-typescript-types. What's happening?

curl -I https://cdn.pika.dev/fp-ts | grep -i x-typescript-types
curl -I https://cdn.pika.dev/@tsfun/pipe | grep -i x-typescript-types
curl -I https://cdn.pika.dev/@tsfun/all | grep -i x-typescript-types
FredKSchott commented 4 years ago

I can see the headers just fine, but with curl -v instead of curl -I. Also in Deno and the browser.

 curl -v https://cdn.pika.dev/fp-ts
// ...
< x-import-url: /-/fp-ts@v2.5.3-ryjcXA22li16wd7sbqeF/dist=es2017/fp-ts.js
< x-pinned-url: /pin/fp-ts@v2.5.3-ryjcXA22li16wd7sbqeF/fp-ts.js
< x-typescript-types: /-/fp-ts@v2.5.3-ryjcXA22li16wd7sbqeF/dist=es2017,mode=types/index.d.ts

I don't know enough about curl to know the difference and why it would show with one but not the other, but are you sure that this was working before?

KSXGitHub commented 4 years ago

curl -i works too.

but are you sure that this was working before?

Yes, last time I tried (curl -I) was about a month ago.

FredKSchott commented 4 years ago

huh. Well, I'm stumped 😅. Any ideas why that could be?

KSXGitHub commented 4 years ago

huh. Well, I'm stumped . Any ideas why that could be?

I don't have any idea either. The best guess I can make is curl somehow changed since I upgraded my system yesterday.

FredKSchott commented 4 years ago

Oh! i think i found it in the manual:

     -I, --head
              (HTTP FTP FILE) Fetch the headers only! HTTP-servers feature the command HEAD which this  uses  to  get
              nothing but the header of a document. When used on an FTP or FILE file, curl displays the file size and
              last modification time only.

We should implement a HEAD method. I'm not sure if we were doing that before tho, but it sounds like that's what is up.

KSXGitHub commented 4 years ago

I see now. I just try again but with -X GET and it works:

curl -I -X GET https://cdn.pika.dev/@tsfun/all | grep -i x-typescript-types
shian15810 commented 4 years ago

@FredKSchott

Regarding this X-TypeScript-Types header:

curl -I -X GET https://cdn.pika.dev/semver

Returned this in the header:

x-typescript-types: /-/semver@v7.1.3-ADApQhGtlqxCaVKMLXbi/dist=es2017,mode=types/index.d.ts

But subsequent curl to this d.ts file returned 502:

curl -I -X GET https://cdn.pika.dev/-/semver@v7.1.3-ADApQhGtlqxCaVKMLXbi/dist=es2017,mode=types/index.d.ts

However, @types/semver is a valid package on https://npmjs.com.

Maybe the d.ts file conversion somehow failed or did not exist in the CDN?

If that so, is it possible to not include it in the X-TypeScript-Types header in the first request?

KSXGitHub commented 4 years ago

@shian15810 SemVer does not support ESM anyway, so it does not matter much. I recommend you to use these packages instead, the blue "TS" bag indicates that the package has TypeScript support.

I also recommend you to use https://pika.dev to search for packages, so that you may know which one supports ESM and TypeScript.

shian15810 commented 4 years ago

@KSXGitHub

Yeah, thanks for the info!

The content of https://cdn.pika.dev/-/semver@v7.1.3-ADApQhGtlqxCaVKMLXbi/dist=es2019/semver.js is actually valid ESM, can be imported by both deno and node ("type": "module").

Only that the 502 is causing deno to crash:

error: Uncaught Error: Import 'https://cdn.pika.dev/-/semver@v7.1.3-ADApQhGtlqxCaVKMLXbi/dist=es2017,mode=types/index.d.ts' failed: 502 Bad Gateway

Also, pouchdb is valid ESM according to https://www.pika.dev/search?q=pouchdb.

However, importing it in deno is causing deno to crash too:

Import 'https://cdn.pika.dev/-/pouchdb@v7.2.1/dist=es2019/pouchdb.js' failed: 500 Internal Server Error

Viewing this URL in browser is showing: Build Error: This package could not be built.

Anyway, both 500 and 502 are harmful to Pika's backend, could possibly lead to server crash.

KSXGitHub commented 4 years ago

@shian15810 It seems there are 2 issues to create, one for Pika (as the issue you are describing is not related to this issue), and one for Deno (as Deno shouldn't crash on error).

shian15810 commented 4 years ago

Actually, deno has nothing to do with this, it should just crash. Just like if you do import unknown from 'https://not/valid/url.ts', it should just crash telling you that the URL couldn't be imported.

I will create another issue for the 502.

Thanks!

KSXGitHub commented 4 years ago

it should just crash

I thought you were talking about Deno's Rust code crashing, but re-reading your comment, I realized that it throws an error. Then it is the correct behavior. However, this behavior [crashing when TypeScript definition is not found] is only desired when developing TypeScript, as type definition are not essential to JavaScript. So I think you should still open an issue on Deno titled "For pure JavaScript code, Deno should not crash when TypeScript definition is not found".

FredKSchott commented 4 years ago

on the pika CDN side of things, this has been fixed! Try again with Deno (and the --reload flag) just in case to confirm.

502 error meant that there was something bad going on on our CDN. It came down to this line:

// https://unpkg.com/browse/@types/semver@7.1.0/index.d.ts
import semverParse = require('./functions/parse');

That's not valid ESM, so our ESM import parser failed. Doing some magic now on our end to handle this as if it were a valid import statement. Thanks for filing!

shian15810 commented 4 years ago

@FredKSchott Thanks for the fast resolution! Keep up the amazing work!

@KSXGitHub Agreed with you! I will create an issue on deno at free time.