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

Declares types for typescript #2

Merged
merged 1 commit into from
Jun 19, 2018
Merged

Conversation

dvc94ch
Copy link
Contributor

@dvc94ch dvc94ch commented Jun 16, 2018

No description provided.

@codecov-io
Copy link

codecov-io commented Jun 16, 2018

Codecov Report

Merging #2 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #2   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files           1        1           
  Lines         222      222           
=======================================
  Hits          221      221           
  Misses          1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a216d6f...7181b06. Read the comment docs.

@@ -0,0 +1,52 @@
declare module 'prettier-printer' {
interface IDoc {}
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm... I'm not intimately familiar with TypeScript, but given TypeScript's use of structural typing, doesn't this mean that any object is seen as a valid document? IOW, given this definition, does e.g. render(0, {foo: 'bar'}) give a type error (it probably should, because it is an error)?

@polytypic
Copy link
Owner

Thanks! This looks basically good, but the definition

interface IDoc {}

seems suspicious to me, because to me it looks like any object would then be accepted as a document, which should not be the case — assuming one actually wants some amount of type safety. I'm not intimately familiar with TypeScript, however, so I might be missing something.

@polytypic
Copy link
Owner

polytypic commented Jun 19, 2018

Perhaps a definition like this would be a good enough approximation:

type IDoc = string | IDocArray
interface IDocArray extends Array<IDoc> { }

This should allow one to pass strings and arrays of documents as documents, but not arbitrary objects or numbers, for example. Of course, there are also other kinds of documents, but one should generally treat documents as abstract and never manipulate them except by using the functions from the library.

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Jun 19, 2018

assuming one actually wants some amount of type safety

type IDoc = string | IDocArray
interface IDocArray extends Array<IDoc> { }

Much better, thank you. Updated it with your suggestion. Also helps with documentation and text editor support.

@polytypic
Copy link
Owner

Looks good. I'll try to find time to merge this and release a new version with the typings this evening. Thanks again for the PR!

@polytypic polytypic merged commit f940ffa into polytypic:master Jun 19, 2018
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.

3 participants