TypeScript support #136

xg-wang · 2018-10-28T07:01:40Z

~~Document TypeScript usage as suggested by @DanielRosenwasser at #72 (comment).~~

~~Provide a build time detection for tsconfig.json as a hint user needs the type support.~~

This PR installs ember-cli-typescript and add typescript support doc to README.

Thanks @mike-north for the discussing the type resolution and detection strategy.

index.d.ts

mike-north

you should also create a tsconfig.json like this

{
  "compilerOptions": {
    "noEmit": true,
    "lib": [],
    "types": []
  },
  "files": ["index.d.ts"]
}

mike-north · 2018-11-02T05:39:27Z

@xg-wang I filed a bug for the NPM issue we ran into https://npm.community/t/installing-dependency-under-a-custom-name-fails-silently/3104

xg-wang · 2018-11-02T06:04:55Z

Great to know there's a pending PR in npm to fix the issue (which I'll document more about when this PR proceeds).
npm/cli#3

mike-north

@xg-wang - ok, I have this branch passing now with a very relaxed TS compiler configuration.

Suggested next steps:

Add obvious types, turn on compiler config "noImplicitAny": true, and make implicit anys explicit. Make another commit and ensure it's green
Try to get rid of explicit anys in small commits, keeping tests green
- Even while developing, keep tests running (i.e., ember test --server) while developing. Sometimes seemingly innocent changes (i.e., changing a if (x) to if (x !== null) can cause things to break and make it very difficult to find the change that needs to be removed.
More strictness in compiler config

	"noImplicitThis": true,
    "noImplicitReturns": true,
    "strictFunctionTypes": true,
    "pretty": true,
    "stripInternal": true,
    "strictNullChecks": true,
    "strict": true,

Add a typescript linting package of some sort (ESLint has a typescript plugin, or TSLint works well too).

addon/mixins/adapter-fetch.ts

xg-wang · 2019-02-01T01:18:01Z

@mike-north can you review again?

mike-north · 2019-02-04T17:38:25Z

README.md

@@ -35,6 +35,12 @@ Available imports:
 import fetch, { Headers, Request, Response, AbortController } from 'fetch';
 ```

+### Use with TypeScript
+To use `ember-fetch` with TypeScript or enable editor's type support, add `"fetch": "ember-cli/ember-fetch"` to your app's `devDependencies`.
+This will get the current state of `ember-fetch` from this GitHub repo as a dependency.


I'm not sure we want to advise that people do this, since they'll end up tracking the master branch rather than SemVer versioned releases.

I tend to document tsconfig.json only.

please clarify. This pertains to package.json, not tsconfig.json

add "fetch": "ember-cli/ember-fetch" to your app's devDependencies.
This will get the current state of ember-fetch from this GitHub repo as a dependency.

I removed the "fetch": "ember-cli/ember-fetch" instruction and only left a tsconfig example

README.md

addon/ajax.ts

addon/mixins/adapter-fetch.ts

mike-north · 2019-02-04T17:48:18Z

addon/types.ts

+}
+
+export interface PlainHeaders {
+  [key: string]: string | undefined | null;


Suggested change

[key: string]: string | undefined | null;

[key: string]?: string | null;

I think typescript doesn't let me do this.

you'll have to change it to a type instead of interface

interface Foo{ bar: string; baz: number; } type A<T> = { [K in keyof T]?: T[K] | null } type X = A<Foo>;/* { bar?: string | null | undefined; baz?: number | null | undefined; } */

https://typescript-play.js.org/#code/JYOwLgpgTgZghgYwgAgGIHt0G8BQz-IBGcUAXMgM5hSgDmA3HgcQF7kgCuAtodIwL44cYAJ4AHFAEEAPABUAfMgC8yLMgDaAaWShkAawgj0MZLIC6AfnKytZ5AB9knADbPkg4eJQANZchkY6PL0APQAVDi4BEQkVpTUdA5OHK5JHCAAJhAwoBAZjNGscZw80Ekubo7pWTkgeQI4YSE4QA

I changed PlainHeaders and PlainObject from interface to type, and made PlainObject more generic.

mike-north · 2019-02-04T17:50:37Z

addon/types.ts

+>;
+
+export function isPlainObject(obj: any): obj is PlainObject {
+  return Object.prototype.toString.call(obj) === '[object Object]';


this isn't really the right implementation for this guard, since it will pass for lots of objects that aren't PlainObject. For example { foo() { return 'bar'; } } will still coerce to string as [object Object]

I want to preserve all current behaviors so it be a type only PR. (I assume Object.prototype.toString.call(obj) equals String(obj). There exists ways to break this but we can leave it in another fix PR.

addon/utils/mung-options-for-fetch.ts

mike-north · 2019-02-04T17:53:45Z

addon/utils/serialize-query-params.ts

+export function serializeQueryParams(
+  queryParamsObject: object | string
+): string {
+  var s: any[] = [];


Suggested change

var s: any[] = [];

const s: any[] = [];

There're lots of var here because it was copy-pasted from jquery code. We can do a refactor in the future and leave this PR a type only one.

types/dummy/index.d.ts

xg-wang

Updated a commit, only thing I need think more about is isPlainObject function signature.

- simplify readme type instruction - drop redundant async in ajax.ts - add credentials to FetchOptions type - add FetchAdapter interface for our object literal and better support Mixin.create typing

mike-north · 2019-02-07T18:43:05Z

Looks like you've responded to all my feedback 👍. This looks good to me!

xg-wang requested a review from mike-north October 28, 2018 07:01

mike-north requested changes Oct 31, 2018

View reviewed changes

index.d.ts Outdated Show resolved Hide resolved

index.d.ts Outdated Show resolved Hide resolved

index.d.ts Outdated Show resolved Hide resolved

mike-north requested changes Oct 31, 2018

View reviewed changes

xg-wang force-pushed the ts branch from 2c72e22 to 5d7ba53 Compare October 31, 2018 23:12

mike-north approved these changes Nov 2, 2018

View reviewed changes

xg-wang force-pushed the ts branch from d0267a2 to aac49e8 Compare November 5, 2018 05:39

install ember-cli-typescript, convert files to .ts

7b95d31

xg-wang force-pushed the ts branch from aac49e8 to 9ae68f6 Compare January 28, 2019 07:13

Add types and tslint

1b328e3

xg-wang force-pushed the ts branch from 9ae68f6 to 1b328e3 Compare January 29, 2019 08:18

xg-wang changed the title ~~TypeScript def file and build time detection~~ WIP: TypeScript support Jan 29, 2019

xg-wang commented Feb 1, 2019

View reviewed changes

addon/mixins/adapter-fetch.ts Outdated Show resolved Hide resolved

mike-north reviewed Feb 4, 2019

View reviewed changes

xg-wang commented Feb 6, 2019

View reviewed changes

xg-wang force-pushed the ts branch 2 times, most recently from ecfbcdb to 89f9b69 Compare February 7, 2019 16:43

More type changes

c64bb4c

- simplify readme type instruction - drop redundant async in ajax.ts - add credentials to FetchOptions type - add FetchAdapter interface for our object literal and better support Mixin.create typing

xg-wang force-pushed the ts branch from 89f9b69 to c64bb4c Compare February 7, 2019 16:49

xg-wang changed the title ~~WIP: TypeScript support~~ TypeScript support Feb 7, 2019

xg-wang merged commit c0ea3a1 into ember-cli:master Feb 8, 2019

xg-wang deleted the ts branch February 8, 2019 19:01

xg-wang added enhancement documentation and removed documentation labels Mar 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeScript support #136

TypeScript support #136

xg-wang commented Oct 28, 2018 •

edited

mike-north left a comment

mike-north commented Nov 2, 2018 •

edited

xg-wang commented Nov 2, 2018

mike-north left a comment •

edited

xg-wang commented Feb 1, 2019

mike-north Feb 4, 2019

xg-wang Feb 6, 2019

mike-north Feb 6, 2019

xg-wang Feb 7, 2019

mike-north Feb 4, 2019

xg-wang Feb 6, 2019

mike-north Feb 6, 2019

xg-wang Feb 7, 2019

mike-north Feb 4, 2019

xg-wang Feb 6, 2019

mike-north Feb 4, 2019

xg-wang Feb 6, 2019

xg-wang left a comment

mike-north commented Feb 7, 2019

	[key: string]: string \| undefined \| null;
	[key: string]?: string \| null;

TypeScript support #136

TypeScript support #136

Conversation

xg-wang commented Oct 28, 2018 • edited

mike-north left a comment

Choose a reason for hiding this comment

mike-north commented Nov 2, 2018 • edited

xg-wang commented Nov 2, 2018

mike-north left a comment • edited

Choose a reason for hiding this comment

xg-wang commented Feb 1, 2019

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

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

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

xg-wang left a comment

Choose a reason for hiding this comment

mike-north commented Feb 7, 2019

xg-wang commented Oct 28, 2018 •

edited

mike-north commented Nov 2, 2018 •

edited

mike-north left a comment •

edited