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

feat: output type declaration #118

Merged
merged 1 commit into from
Oct 13, 2021
Merged

feat: output type declaration #118

merged 1 commit into from
Oct 13, 2021

Conversation

dpilch
Copy link
Member

@dpilch dpilch commented Oct 11, 2021

  • add option to output TypeScript type declarations
  • change generated component type to React.Element
    • JSX.Element was causing errors when trying to compile type declarations

I used ts.createProgram to compile the type declarations. Our existing solution of ts.transpileModule does not support creating type declarations. ts.createProgram requires the source to be written to the filesystem. ts.createProgram is much less performant than ts.transpileModule so ts.createProgram is only used when necessary.

@dpilch dpilch requested a review from alharris-at October 11, 2021 15:18
@@ -7,6 +7,7 @@ export type ReactRenderConfig = FrameworkRenderConfig & {
script?: ScriptKind;
target?: ScriptTarget;
module?: ModuleKind;
declaration?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's probably a more readable or understandable name for this param. Maybe like exportTypeDeclarations?

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 was following the naming convention from TypeScript, but you are right that it isn't very descriptive. I'll change to something else.

Comment on lines 34 to 35
// createProgram is less performant than transpileModule.
// createProgram should only be used when necessary.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is written more like an either-or, rather than a 'don't generate d.ts files if we don't need to, it's an expensive operations'.

Comment on lines 49 to 50
tmpFile.removeCallback();
tmpDir.removeCallback();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be handled in a try-catch-finally to ensure we do our best effort to release any resources we're grabbing along the way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, checking the source it looks like these could throw errors. Looks like tmp.fileSync and tmp.dirSync could throw errors as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I mean if the fs module throws, then that seems reasonable that we didn't release some handle. I think more likely if our config or import code causes createProgram to throw then we should probably do some cleanup.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, I've updated so that the tmp files should always be cleaned up.

@dpilch dpilch force-pushed the output-declarations branch from e5b46c9 to b1a9e66 Compare October 12, 2021 19:46
@dpilch dpilch changed the title feat: output type declartion feat: output type declaration Oct 12, 2021
@dpilch dpilch force-pushed the output-declarations branch from b1a9e66 to a8db227 Compare October 12, 2021 20:57
@dpilch dpilch merged commit 9db8bdc into main Oct 13, 2021
@dpilch dpilch deleted the output-declarations branch October 13, 2021 14:05
alharris-at added a commit that referenced this pull request Oct 15, 2021
alharris-at added a commit that referenced this pull request Oct 15, 2021
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