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

Add TypeScript support to Mirage.js #225 #262

Merged
merged 63 commits into from
Apr 1, 2020

Conversation

zoltan-nz
Copy link
Contributor

#225

An initial draft to add TypeScript support to Server class.

Any feedback is welcome. If you have any question or suggestion please don't hesitate to comment here.

types/index.d.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@asantos00 asantos00 left a comment

Choose a reason for hiding this comment

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

IMHO I think these types already help a lot whoever is using TypeScript with mirage.

I think we can then grow from this and start adding types to crucial things like schema and db

types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
.eslintignore Outdated Show resolved Hide resolved
@zoltan-nz
Copy link
Contributor Author

@asantos00 Thanks for your suggestions, really appreciate. I just implemented those. You right, schema and db would be a great addition... It would be also great if we can use Mirage in Angular applications. ;)

types/index.d.ts Outdated Show resolved Hide resolved
types/mirage-type-test.ts Outdated Show resolved Hide resolved
@samselikoff
Copy link
Contributor

This looks pretty awesome guys 👍Thanks so much for this work @zoltan-nz!

I'm unfamiliar with TS though it is high on my priority list. It looks like this makes typescript a dependency of Mirage. Does that add a lot to install size or affect non-TS users in other ways?

I'd also like to find other folks in the Ember ecosystem using Mirage with TS, and get their take on this. I know @chriskrycho has fiddled with this before.

@chriskrycho
Copy link
Contributor

Thanks for the ping! I'll carve out some time tomorrow to dig into these. And thanks @zoltan-nz for starting this work. cc. also @jamescdavis @dustinsoftware, both of whom are (I believe) working on apps making active use of Mirage with TS today.

@chriskrycho
Copy link
Contributor

Hey folks, I need to punt this to slightly later in the week. I will say that my strongest recommendation is: we need type tests for this, using tsd. We want to make sure that the API actually matches what we say it is, and we also want to make sure that the types shipped don't suddenly widen in ways that allow illegal things between versions.

(I'm hopefully going to be working on some standard infrastructure to make this easy over the course of the year, but this could be a great testbed for figuring out what makes for a good experience with that.)

@chriskrycho
Copy link
Contributor

chriskrycho commented Jan 9, 2020

For folks following along at home, when I do dig into this (🤞 hopefully Friday 🤞) I'm going to be referring to the following type definitions that @dfreeman, @jamescdavis, and I developed independently over the last few years, so that what we end up with is hopefully a best-of approach that includes the work done by @zoltan-nz, @asantos00, and the three of us:

@dfreeman
Copy link
Contributor

dfreeman commented Jan 9, 2020

Awesome, thanks for pulling all this together @chriskrycho! I just updated that gist to include our type tests as well—they're written against dtslint rather than tsd, but I know you're familiar with reading that style too 😜

@asantos00
Copy link
Collaborator

asantos00 commented Jan 9, 2020

Maaan, this is really cool. There are a lot of written types that I didn't knew about, which is amazing 🙏 . I think it's mostly a question of merging/choosing one. The hardest part will definitely be the types tests, which I'm not super aware of but will have a look

@asantos00
Copy link
Collaborator

asantos00 commented Jan 10, 2020

I was reading @dfreeman types and I think they're pretty complete. Do you think it makes sense to add them to this PR (or another one) so we can iterate on them?

@jamescdavis
Copy link
Contributor

Thanks for getting this going @zoltan-nz and @asantos00! This may not be relevant here because this is miragejs and no longer ember-specific, but something the ember-osf-web types do (that might be a bit crazy) is ensure that mirage factories and fixtures are type-compatible with the ember-data model they are based on. Here's a fairly simple factory that makes use of this. You can see that it:

import Comment from 'ember-osf-web/models/comment';

defines an interface of traits:

export interface CommentTraits {
    withReplies: Trait;
    asAbuse: Trait;
}

and then passes their intersection as a type arg:

export default Factory.extend<Comment & CommentTraits>({

The type arg ensures that properties set in the Comment factory match type with the ember-data Comment model (or are a trait defined in CommentTraits).

To do this requires some pretty interesting mapped types. This may be overkill to try to support this for the first version of these types (and maybe not something we ever want to try to support in the "official" types), but just some food for thought.

@zoltan-nz
Copy link
Contributor Author

It looks to me, that dtslint behaves strangely. I tried it on macOS and inside a Linux docker container. Throw errors, like TypeError: Cannot read property 'length' of undefined

The 'member-access' rule threw an error in '/home/node/app/types/index.d.ts':
TypeError: Cannot read property 'length' of undefined
    at recur (/home/node/app/node_modules/dtslint/node_modules/tslint/lib/rules/memberAccessRule.js:91:57)
    at visitNodes (/home/node/app/node_modules/dtslint/node_modules/typescript/lib/typescript.js:18253:30)
    at Object.forEachChild (/home/node/app/node_modules/dtslint/node_modules/typescript/lib/typescript.js:18484:24)
    at recur (/home/node/app/node_modules/dtslint/node_modules/tslint/lib/rules/memberAccessRule.js:108:19)
    at visitNode (/home/node/app/node_modules/dtslint/node_modules/typescript/lib/typescript.js:18244:24)
    at Object.forEachChild (/home/node/app/node_modules/dtslint/node_modules/typescript/lib/typescript.js:18585:21)
    at recur (/home/node/app/node_modules/dtslint/node_modules/tslint/lib/rules/memberAccessRule.js:108:19)
    at visitNodes (/home/node/app/node_modules/dtslint/node_modules/typescript/lib/typescript.js:18253:30)
    at Object.forEachChild (/home/node/app/node_modules/dtslint/node_modules/typescript/lib/typescript.js:18486:24)
    at walk (/home/node/app/node_modules/dtslint/node_modules/tslint/lib/rules/memberAccessRule.js:89:15)

Also it complains about var keyword, however there isn't any var keyword...

Error: /home/node/app/types/index.d.ts:60:16
ERROR: 60:16    no-var-keyword                   Forbidden 'var' keyword, use 'let' or 'const' instead

Do you guys have any suggestion how could we test these types? It would be great to complete this PR, so miragejs can officially support Angular projects as well. ;)

@samselikoff
Copy link
Contributor

Kinda keeping one eye on this PR, I'm glad you all are collaborating and I don't want to be a bottleneck.

I still don't have TS experience but this is a big PR. I know some projects have a separate @types repo that contains the types.

Would it be helpful if we moved this to an official @miragejs/types repo? Would it help you all iterate + discuss it better?

@samselikoff
Copy link
Contributor

I think we could update node in Travis, it’s pretty unlikely folks will hit node errors with Mirage. I’m totally OK with that if it makes your life easier.

@zoltan-nz
Copy link
Contributor Author

Actually, I realised, the qunit method is not exist in the official API, so I removed it. Importing qunit type definition is also problematic, because different projects use different test runner (e.g jest).

@zoltan-nz
Copy link
Contributor Author

Should we add or change anything in this PR before merging?

@maciejmyslinski
Copy link

maciejmyslinski commented Mar 10, 2020

this.resource is not covered 🙈
I'd recommend handling that in the next PR, though.
BTW I couldn't find this.resource mentioned in the API documentation neither.

It's in other parts of the documentation, thought https://miragejs.com/docs/testing/application-tests/#our-first-test

@WonderPanda
Copy link

I know that +1 style comments can be a pain some times but I just wanna echo that I'm eagerly waiting for this to get merged. MirageJS looks amazing and I want to start adopting it into multiple code bases but it's hard when it's the only part of my front end stack that isn't typed

@samselikoff
Copy link
Contributor

@WonderPanda Understood and no worries, it's helpful to know who all is waiting on this.

This PR is unique in that I am basically unsuited to be responsible for it, as TS is currently so far outside of my domain. I know it sucks to wait but Dan and Chris are the folks I've chosen to help audit Zoltan's fantastic work, and so we'll just have to wait until they can carve out some time.

I believe Chris' last comment was to get some input from Dan.

In the meantime, if you did want to start using this, you could give this PR a dry run in your project. You might need to run yarn build after installing it from github since there is a build step. Let me know if you need help.

@dfreeman
Copy link
Contributor

@chriskrycho and I connected on this the other day and should hopefully be able to get things moving again shortly!

@glinesbdev
Copy link

I'm also waiting on this PR. Feel free to reach out if there is help needed here.

@Marcus-Rise
Copy link

I'm too waiting types for this package

types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
@chriskrycho
Copy link
Contributor

Thank you for your patience – had a bunch of things block me from getting back to this, but I have at last! There's one last pair of small changes to make, and after that let's ship this thing!

zoltan-nz and others added 3 commits April 1, 2020 13:30
Co-Authored-By: Chris Krycho <hello@chriskrycho.com>
Co-Authored-By: Chris Krycho <hello@chriskrycho.com>
Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

:shipit:

@samselikoff samselikoff merged commit 5010767 into miragejs:master Apr 1, 2020
@samselikoff
Copy link
Contributor

🚀 Amazing team work guys!!! So happy to see you all come together on this.

Tried to get you three in as co-authors on the commit and it looks like it worked. Thanks again for your work on this – I know tons of people can't wait to try it!

I'll cut a release very soon 👍

@zoltan-nz
Copy link
Contributor Author

zoltan-nz commented Apr 1, 2020

@samselikoff Oh, wow. You are not joking... ;) (It is already 1st of April in New Zealand.)

I'm sure there gonna be a few follow up update and refinement. I try to push them through now. Keep you posted.

It's my pleasure to work with you guys in this beautiful and super useful project. :)

@samselikoff
Copy link
Contributor

Haha no joking here!

Anyway we don’t have to cut a release till you feel good, but I saw ship it so I shipped!

@zoltan-nz
Copy link
Contributor Author

Linter started to complain on the upcoming version of TypeScript. I just opened a ticket and let's solve it quickly. #382

@jamescdavis
Copy link
Contributor

These types don't typecheck, even on 3.8:

types/index.d.ts:214:20 - error TS2344: Type 'Registry' does not satisfy the constraint 'Record<string, { id?: string; attrs: Record<string, unknown>; modelName: string; save(): void; update<K extends never>(key: K, value: {}[K]): void; update(changes: Partial<{}>): void; destroy(): void; reload(): void; }>'.

214     schema: Schema<Registry>,
                       ~~~~~~~~

please delay shipping until #383 (which I'm reviewing now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.