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

Fix TypeScript Definitions for making lit-element happy #46

Merged
merged 1 commit into from Jun 4, 2019

Conversation

mistyharsh
Copy link
Contributor

This PR fixes issue #37.

packages/mixins/apollo-element-mixin.d.ts Outdated Show resolved Hide resolved
packages/lit-apollo/apollo-query.d.ts Outdated Show resolved Hide resolved
@bennypowers
Copy link
Member

Thank you so much for the PR. Please see review comments.

@mistyharsh
Copy link
Contributor Author

@bennypowers Thank you for the review. I did think about these issues before raising the PR. I just hoped to fix TypeScript issues quickly. I will work it out these comments and see how we can adhere to the design.

mistyharsh added a commit to mistyharsh/apollo-elements that referenced this pull request Apr 12, 2019
1. Remove LitElement dependency from mixins package
2. Use class and interface declaration merging for extending from LitElement
@mistyharsh
Copy link
Contributor Author

mistyharsh commented Apr 12, 2019

So there are a couple of things:

  • After reading Anders' comment, It seems it is almost impossible to solve this problem. The only option is to use class and interface merging.
  • ApolloMutation cannot be currently fixed unless we change update property as it conflicts with LitElement.update() method. We should implement RFC RFC: Rename update property to updateCache #35.
  • With declaration merging, we get empty interfaces which fail the linting rules.

@bennypowers
Copy link
Member

I'm ok with implementing #35, although it will represent a major release.

What I'd love to see would be some tests for these typings.

Another option we should consider is using JSDoc inline typings instead of .d.ts files.

@mistyharsh
Copy link
Contributor Author

mistyharsh commented Apr 15, 2019

Having Tests for typescript definition files is not very common. We can treat our files and run tsc to validate our type definitions the way we run linter for our JS code. That is something I can think of. We can also probably look at dtslint though I have not really used it.

As far as JSDoc is concerned, it looks interesting but usually ends up being cumbersome to write than plain TS definitions.

@RSWilli
Copy link

RSWilli commented Jun 3, 2019

what is the status for this? I am having the same issue with TS and lit-apollo.

This PR doesn't just "make lit-element happy", currently TS just doesn't compile the code because it complains that none of the apollo properties (client, query, etc.) are present on my class

@bennypowers
Copy link
Member

@RSWilli can you send me a repro to tinker with? My volunteer time is short this week as I just moved apartment, but I'm really hoping to solve this for you ASAP, so if you have a ready made example repo, or some changes for this PR I'll try my best to get something out.

@RSWilli
Copy link

RSWilli commented Jun 4, 2019

Repro: https://stackblitz.com/edit/typescript-hfggxi

for me, editing https://github.com/apollo-elements/apollo-elements/blob/master/packages/lit-apollo/apollo-query.d.ts#L1 from @apollo-elements/mixins/apollo-mutation-mixin to @apollo-elements/mixins/apollo-query-mixin fixes it, but I don't know if it brakes anything else that my usecase does not include

affects: @apollo-elements/lit-apollo
@bennypowers bennypowers merged commit 0fc03de into apollo-elements:master Jun 4, 2019
@bennypowers
Copy link
Member

very nice work, thanks

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.

None yet

3 participants