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

Added typescript compiler to generate typescript definitions #22

Merged
merged 4 commits into from
Sep 13, 2021
Merged

Added typescript compiler to generate typescript definitions #22

merged 4 commits into from
Sep 13, 2021

Conversation

andersthorsen
Copy link
Contributor

@andersthorsen andersthorsen commented Sep 8, 2021

Hello!

I took the liberty of adding typescript definitions to this project.

To do this I added the typescript compiler and configured to output typescript definitions. It does this by inferring the types from react (@types/react @types/react-dom).

In addition I added JSDoc annotation to the props as it did not infer those by itself

I also had to adjust the constructor a bit by adding the props as a explicit parameter.

(Ref #3)

Would you accept this pull request?

As an alternative I could convert the code to typescript if you wish. In essence, typescript is just modern javascript (ecmascript 2020) with types added. During compliation process the typescript compiler validates the types and then throws them away.

babel also supports typescript but I belive it simply ignore the type definitions.

@apearce
Copy link
Owner

apearce commented Sep 9, 2021

Hi Anders,

Thanks for doing this. I have been meaning to look into this for a long time. Will this impact non typescript users at all?

Alan

@andersthorsen
Copy link
Contributor Author

I don't see how this should impact non-typescript users negatively. It's just .d.ts files next to the .js file with type information.

If non-ts users use an editor like VS Code they will get better suggestions/documentation (IntelliSense) as several editors will still utilize the .d.ts files for this purpose even for .js files.

My main concern would be if the two code changes I had to make has any impact that I can't see.

@apearce
Copy link
Owner

apearce commented Sep 10, 2021

Great. I should have some time this weekend to do a little testing and assuming all goes well I'll merge it in.

Thanks again.

@apearce
Copy link
Owner

apearce commented Sep 12, 2021

Hi Anders,

I did a little testing of the demo page and your code changes didn't have any impact.

I have a question about the generated index.d.ts file. This is what I see generated:

export default ReactShadowRoot;
import ReactShadowRoot from "./ReactShadowRoot.js";

I surprised having the export before the import works but I tested it elsewhere and it does. But my question is, and excuse my typescript ignorance, shouldn't that import the ReactShadowRoot.d.ts file?

Could you also update the README? Just a new item in the note section saying "Type definitions included" should be fine.

@andersthorsen
Copy link
Contributor Author

Hi Anders,

I did a little testing of the demo page and your code changes didn't have any impact.

I have a question about the generated index.d.ts file. This is what I see generated:

export default ReactShadowRoot;
import ReactShadowRoot from "./ReactShadowRoot.js";

I surprised having the export before the import works but I tested it elsewhere and it does.
Good question!

I guess this answers why this works:
https://exploringjs.com/es6/ch_modules.html#_imports-are-hoisted

Why the TypeScript compiler does this order is beyond me. I'd also do it in reverse if I did it manually. The reason for doing it via the typescript compiler is that I feel that scales better as it will then adjust if the signature is adjusted.

But my question is, and excuse my typescript ignorance, shouldn't that import the ReactShadowRoot.d.ts file?

My understanding is that the .d.ts files are a kind of "overlay" of their .js countepart.

I.e. that when you import "x.js", if x.d.ts exists then that will be overlaid with regards to the type information. Again I'm not a TypeScript expert on exactly why the generated code is like this, but when I go to VS Code and click on the ReactShadowRoot reference in index.d.ts it sends me off to ReactShadowRoot.d.ts.

Could you also update the README? Just a new item in the note section saying "Type definitions included" should be fine.

Sure.

@apearce apearce merged commit 83921e8 into apearce:master Sep 13, 2021
@apearce
Copy link
Owner

apearce commented Sep 13, 2021

Merged. Thanks again for doing this

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