Skip to content

Adds typescript .d.ts declaration file for RegularTableElement#56

Closed
telamonian wants to merge 3 commits intomasterfrom
add-ts-declarations
Closed

Adds typescript .d.ts declaration file for RegularTableElement#56
telamonian wants to merge 3 commits intomasterfrom
add-ts-declarations

Conversation

@telamonian
Copy link
Copy Markdown
Contributor

@telamonian telamonian commented Jun 15, 2020

  • Adds dist/esm to the build. This includes .d.ts files autogenerated from the existing jsdoc comments in the javascript sources (using the standard tsc typescript compiler)
  • fixup/add missing jsdoc comments
  • add manually curated index.d.ts file to project root

@telamonian telamonian force-pushed the add-ts-declarations branch from 79ed268 to 966a19c Compare June 15, 2020 20:48
Copy link
Copy Markdown
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

This is a nice addition but needs some work. I've started some of these changes in ths ts-wip branch, but there is still work left (the sections below the break).

  • Revert element.js rename please to index.js, don't need a separate file just to register this class nor should the class itself be exported as it is not intended to be inherited.
  • Remove docs non-public methods 'get_tds()', et al methods removed by file-browser-rewrite branch, only styleListener() is public and missing docs.
  • Remove esm output entirely - removing the need for these multiple output targets for different module systems was the entire motivation behind moving to rollup. Use emitDeclarationOnly flag and skip js emit entirely, all we need are the .d.ts for the module itself.
  • Don't emit .d.ts for anything but index.js - the other files are not even exported by the module.

Not fixed in ts-wip:

  • Fix e.g. _column_sizes, _styles_callbacks private methods to not emit types.
  • Generate index.d.ts directly from build, having this as a separate copy/paste asset with hand-written extensions kindof offsets the purpose of generating these (certainly --watch-ing them).
  • Speaking of which - can JSDoc outputs handle the Intrinsic and React type hints? Let's please figure out a way to template these as part of index.d.ts generation.

@telamonian
Copy link
Copy Markdown
Contributor Author

  • Revert element.js rename please to index.js, don't need a separate file just to register this class nor should the class itself be exported as it is not intended to be inherited.

I poked around for several hours, but was unable to find a way to generate the declaration for the RegularTableElement class without export-ing it. I moved RegularTableElement to a new element.js file so that I could export it without exposing it externally (since everything export-ed from index.js is treated as a top-level export of the package).

  • Fix e.g. _column_sizes, _styles_callbacks private methods to not emit types.

It was not obvious how to get tsc to ignore these, but there has to be a way to do that, right? If not I guess that's something that should be submitted upstream in the typescript project as a bug.

  • Speaking of which - can JSDoc outputs handle the Intrinsic and React type hints? Let's please figure out a way to template these as part of index.d.ts generation.

I don't think I managed to convey a full appreciation of how limited tsc's capabilities are vis-a-vis declaration generation from javascript sources 😄

I agree that this would be very nice to have, but we'll have to roll our own declaration generation system to support this (maybe as a rollup plugin?)

@telamonian
Copy link
Copy Markdown
Contributor Author

Thanks for the feedback! There was a bunch of stuff that I wasn't sure about (eg the existence of the separate dist/esm build), and I figured the easiest way to have a discussion about it would be to just float this PR.

All of the things you mention under "Not fixed in ts-wip" are things I tried to implement but found that I could not easily do so. I think the reality is that tsc's capabilities vis-a-vis declaration generation are currently extremely limited, and are mostly lacking in terms of configurability. Here's the [docs].

So the approach I went for in the end was basically to just manually write index.d.ts by hand. The .d.ts files generated by the typescript build are then mostly meant to just serve as a convenient starting point, so that index.d.ts doesn't have to be created completely from scratch.

@telamonian
Copy link
Copy Markdown
Contributor Author

Closing this in favor of #65

@telamonian telamonian closed this Jun 18, 2020
@timkpaine timkpaine deleted the add-ts-declarations branch February 15, 2023 03:56
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.

2 participants