Remove hand crafted types for text-encoding #6

mohsen1 · 2018-05-29T16:16:58Z

Still WIP but comments are welcome

This change:

Removes hand written types for text-encoding module and uses @types/text-encoding
Updates the list script to enforce TypeScript type checking and Prettier code lint (must ran Prettier before)

TODO List:

Resolve all tsc issues
- Fix duplicate declaration for global TextDecoder and TextEncoder. It already exist in dom.d.ts
- Figure out typing for *.pb (protobuf?) files. I need help with this. Should we have a declaration per file? Can we use something like Proto2TypeScript to automate it?

fearphage · 2018-05-29T19:15:51Z

.gitignore

@@ -6,3 +6,4 @@ assets.go
 msg.pb.go
 msg.pb.js
 msg.pb.d.ts
+.vscode


FYI, you can ignore this file globally so you don't have to add it to each repo individually.

That's nice and I did it on my machine. I think it's good to have it here too since VSCode is pretty popular these days :)

I find it it best to keep the files in each repository specific to that project instead of about the people editing it. This project has no reliance on or interactions with VS Code. But it all comes down to personal preference in the end.

qti3e · 2018-05-29T19:26:56Z

declarations.d.ts

@@ -0,0 +1,14 @@
+declare module "*.pb" {


protobuf auto-generates this file based on contents inside msg.proto, look at msg.pb.d.ts after running:

make deno

or in case you don't want to build v8 (which takes ~30min)

make msg.pb.d.ts

ry · 2018-05-29T22:26:40Z

@mohsen1 thanks! I applaud the effort. If you can remove text-encoding.d.ts while also not depending on @types/text-encoding, I would be very happy. I tried a bit to do this but got stuck.

CLAassistant · 2018-05-30T05:24:33Z

All committers have signed the CLA.

mohsen1 · 2018-06-01T03:33:26Z

@ry you can't import from "text-encoding" module without having it declared somewhere.

https://github.com/ry/deno/blob/a4d89f2ee1ddde786f2335eada86f00e3efd1174/globals.ts#L54

This is the minimal declaration if you don't like to have a dependency just for types.

yorkie · 2018-06-01T03:40:36Z

tsconfig.json

@@ -1,14 +1,13 @@
 {
 "compilerOptions": {
- "allowJs": true,


Why removing this option?

shoot! I didn't meant to dod that!

yorkie · 2018-06-01T15:25:43Z

runtime.ts

@@ -149,7 +149,7 @@ export function resolveModule(
 let fetchResponse;
 try {
 fetchResponse = os.codeFetch(moduleSpecifier, containingFile);
- } catch(e) {
+ } catch (e) {


This change might be fixed at #73 :)

Unrelated change

qti3e

Thank you Moshen, it looks good to me, I have a few comments...
Also, please squash : )

qti3e · 2018-06-01T15:32:44Z

.gitignore

@@ -6,3 +6,4 @@ assets.go
 msg.pb.go
 msg.pb.js
 msg.pb.d.ts
+.vscode


Unrelated change

qti3e · 2018-06-01T15:32:59Z

package.json

- "lint": "tslint -p tsconfig.json -c tslint.json",
- "fmt": "prettier --write *.ts* *.js *.json"
+ "lint": "tslint -p . && prettier --list-different \"{*.ts,js,json}\"",
+ "fmt": "prettier --write \"{*.ts,js,json}\""


Unrelated change

qti3e · 2018-06-01T15:33:05Z

runtime.ts

@@ -149,7 +149,7 @@ export function resolveModule(
 let fetchResponse;
 try {
 fetchResponse = os.codeFetch(moduleSpecifier, containingFile);
- } catch(e) {
+ } catch (e) {


Unrelated change

ry · 2018-06-01T18:39:50Z

This doesn't solve the TODO. Thanks for researching it. We can revisit later with that TS issue is fixed.

mohsen1 · 2018-06-01T19:06:31Z

I don't understand how it would be possible to import from text-encoding module if you don't want to depend on@types/text-encoding and delete text-encoding.d.ts. Module has to be declared somehow

ry · 2018-06-01T19:14:12Z

It's inside TS: https://github.com/Microsoft/TypeScript/blob/f8503f26328beb8755792528dd312f29f9d5415c/lib/lib.dom.d.ts#L13139-L13142

mohsen1 · 2018-06-01T19:20:59Z

A module with "text-encoding" specifier is not defined in lib.dom.d.ts. If I'm reading the code correctly you are importing a runtime value from the text encoding module and assigning it to global object.

ry · 2018-06-01T19:27:52Z

Right.. so somehow we need to import the implementation (polyfill) but not have typescript complain about types missing.

mohsen1 · 2018-06-01T19:58:44Z

To import those values from "text-encoding" module there should be a module declaration somewhere. There are two options :

create a minimal module declaration that declares exports of TextEncoder and TextDecoder with types of dom lib. That would be this PR. TypeScript has two variable namespaces. One for values and one for types. That's why I have TextDecoder: TextDecoder
depend on @types/text-encoder which declares the module. I'm trying to fix it to use dom types here use native DOM types for text-encoding module DefinitelyTyped/DefinitelyTyped#26141

ry · 2018-06-01T20:05:56Z

@mohsen1 oh gosh I'm very sorry. I miss read the commit - I thought it was still importing @types/text-encoder.
Thank you for solving this! (And thank you @yorkie and @qti3e for reviewing)

fearphage reviewed May 29, 2018

View reviewed changes

qti3e reviewed May 29, 2018

View reviewed changes

mohsen1 changed the title ~~[WIP] Remove hand crafted types~~ Remove hand crafted types for text-encoding May 30, 2018

yorkie reviewed Jun 1, 2018

View reviewed changes

yorkie approved these changes Jun 1, 2018

View reviewed changes

yorkie reviewed Jun 1, 2018

View reviewed changes

qti3e approved these changes Jun 1, 2018

View reviewed changes

mohsen1 force-pushed the remove-hand-crafted-types branch 2 times, most recently from cdc3f8c to e5f09fe Compare June 1, 2018 16:21

squash

e54ba89

mohsen1 force-pushed the remove-hand-crafted-types branch from e5f09fe to e54ba89 Compare June 1, 2018 16:22

ry closed this Jun 1, 2018

ry reopened this Jun 1, 2018

ry merged commit 7b5fbc7 into denoland:master Jun 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove hand crafted types for text-encoding #6

Remove hand crafted types for text-encoding #6

mohsen1 commented May 29, 2018 •

edited

Loading

fearphage May 29, 2018

mohsen1 May 30, 2018

fearphage May 30, 2018

qti3e May 29, 2018

ry commented May 29, 2018 •

edited

Loading

CLAassistant commented May 30, 2018 •

edited

Loading

mohsen1 commented Jun 1, 2018

yorkie Jun 1, 2018

mohsen1 Jun 1, 2018

yorkie Jun 1, 2018

qti3e Jun 1, 2018

qti3e left a comment

qti3e Jun 1, 2018

qti3e Jun 1, 2018

qti3e Jun 1, 2018

ry commented Jun 1, 2018

mohsen1 commented Jun 1, 2018

ry commented Jun 1, 2018

mohsen1 commented Jun 1, 2018

ry commented Jun 1, 2018 •

edited

Loading

mohsen1 commented Jun 1, 2018

ry commented Jun 1, 2018 •

edited

Loading

Remove hand crafted types for text-encoding #6

Remove hand crafted types for text-encoding #6

Conversation

mohsen1 commented May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry commented May 29, 2018 • edited Loading

CLAassistant commented May 30, 2018 • edited Loading

mohsen1 commented Jun 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qti3e left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry commented Jun 1, 2018

mohsen1 commented Jun 1, 2018

ry commented Jun 1, 2018

mohsen1 commented Jun 1, 2018

ry commented Jun 1, 2018 • edited Loading

mohsen1 commented Jun 1, 2018

ry commented Jun 1, 2018 • edited Loading

mohsen1 commented May 29, 2018 •

edited

Loading

ry commented May 29, 2018 •

edited

Loading

CLAassistant commented May 30, 2018 •

edited

Loading

ry commented Jun 1, 2018 •

edited

Loading

ry commented Jun 1, 2018 •

edited

Loading