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

TypeScript support #136

Merged
merged 3 commits into from
Feb 8, 2019
Merged

TypeScript support #136

merged 3 commits into from
Feb 8, 2019

Conversation

xg-wang
Copy link
Member

@xg-wang xg-wang commented Oct 28, 2018

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 Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@mike-north mike-north left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also create a tsconfig.json like this

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

@mike-north
Copy link
Member

mike-north commented Nov 2, 2018

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

xg-wang commented Nov 2, 2018

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

Copy link
Member

@mike-north mike-north left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested next steps:

  1. Add obvious types, turn on compiler config "noImplicitAny": true, and make implicit anys explicit. Make another commit and ensure it's green
  2. 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.
  3. More strictness in compiler config
	"noImplicitThis": true,
    "noImplicitReturns": true,
    "strictFunctionTypes": true,
    "pretty": true,
    "stripInternal": true,
    "strictNullChecks": true,
    "strict": true,
  1. Add a typescript linting package of some sort (ESLint has a typescript plugin, or TSLint works well too).

@xg-wang xg-wang changed the title TypeScript def file and build time detection WIP: TypeScript support Jan 29, 2019
addon/mixins/adapter-fetch.ts Outdated Show resolved Hide resolved
@xg-wang
Copy link
Member Author

xg-wang commented Feb 1, 2019

@mike-north can you review again?

README.md Outdated
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to document tsconfig.json only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

README.md Outdated Show resolved Hide resolved
addon/ajax.ts Show resolved Hide resolved
addon/mixins/adapter-fetch.ts Outdated Show resolved Hide resolved
addon/mixins/adapter-fetch.ts Outdated Show resolved Hide resolved
addon/types.ts Outdated
}

export interface PlainHeaders {
[key: string]: string | undefined | null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[key: string]: string | undefined | null;
[key: string]?: string | null;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think typescript doesn't let me do this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

>;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
export function serializeQueryParams(
queryParamsObject: object | string
): string {
var s: any[] = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var s: any[] = [];
const s: any[] = [];

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Copy link
Member Author

@xg-wang xg-wang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@xg-wang xg-wang changed the title WIP: TypeScript support TypeScript support Feb 7, 2019
@mike-north
Copy link
Member

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

@xg-wang xg-wang merged commit c0ea3a1 into ember-cli:master Feb 8, 2019
@xg-wang xg-wang deleted the ts branch February 8, 2019 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants