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

[RFC] Ship typescript types for the public APIs #816

Open
stof opened this issue Aug 3, 2020 · 6 comments
Open

[RFC] Ship typescript types for the public APIs #816

stof opened this issue Aug 3, 2020 · 6 comments

Comments

@stof
Copy link
Member

stof commented Aug 3, 2020

Several IDEs (including PHPStorm/WebStorm and VS Code) are using the typescript type definitions to enhance their autocompletion for Javascript as well.
While investigating #815, I made a small experiment: I generated a index.d.ts file in webpack-encore to have type definitions for the Encore public API (which is defined and documented in index.js). And this solved the type support for PHPStorm 2020.2.

Given that the type declaration can be fully generated from the existing JSDoc, I suggest that we bundle it in the package.
For the record, here is the command I ran (at the root of the package): tsc --declaration --allowJs --emitDeclarationOnly index.js.

To make this production-ready, a few changes would be necessary:

  • use a proper semver constraint for the typescript devDependency rather than >=2.9 to be able to use tsc in our publication process (no impact on projects using encore as that constraint is not enforced for enabling typescript features)
  • improve JSDoc types for arrays by specifying the value type (Typescript does not like unspecified Array types).
  • (optionally) improve the JSDoc for callbacks to document their actual signature rather than using {function} (but this will be hard to go fully there: configuration callbacks are dealing with options of loaders, but we cannot refer to the type definitions shipped in these loaders for any optional dependency)
  • (optionally) improve the JSDoc for webpack-encore options to document the available options (for arguments where we mix loader options and Encore options, that's not possible for the same reason than above, but we have many methods with a separate argument for Encore options)
  • add a prepublishOnly script doing that type generation before publishing the releases to npm

All the improve JSDoc steps could actually be done even without shipping typescript type declarations, as they would still proper better type definitions. And actually, I have part of them done locally as part of my experiment.

What do you think @Lyrkan @weaverryan ?

My own vote would be in favor of including them, given that we don't need to maintain the .d.ts file manually.

@stof
Copy link
Member Author

stof commented Aug 3, 2020

Note that even if the IDE decides to take the .d.ts file as the autocompletion source, all the explanations written in the JSDoc are preserved, as tsc will copy the JSDoc to the type declaration.

@stof
Copy link
Member Author

stof commented Sep 11, 2020

@Lyrkan @weaverryan any opinion on this ?

@chapterjason
Copy link

chapterjason commented Sep 11, 2020

@stof

Note that even if the IDE decides to take the .d.ts file as the autocompletion source, all the explanations written in the JSDoc are preserved, as tsc will copy the JSDoc to the type declaration.

You can also reference the .d.ts in the package.json Including declarations in your npm package

In long term I would suggest writing the source in typescript, couldn't find any considerations about that, but I don't think there are any drawbacks.

@stof
Copy link
Member Author

stof commented Sep 11, 2020

You can also reference the .d.ts in the package.json Including declarations in your npm package

that's irrelevant for this comment. My comment does not care about how the d.ts file is discovered by the IDE.

In long term I would suggest writing the source in typescript, couldn't find any considerations about that, but I don't think there are any drawbacks.

that's a totally different topic. And given that Encore deals with tons of optional dependencies, it might be hard to make it typed (without using any in lots of places, defeating the purpose). For now, I'm not convinced of the benefits of rewriting Encore to Typescript. But I am convinced of the benefits of better code completion in IDEs (btw, it will also help people writing the webpack config file in typescript). In any case, that's of-topic for this RFC.

@Lyrkan
Copy link
Collaborator

Lyrkan commented Sep 11, 2020

@Lyrkan @weaverryan any opinion on this ?

That would be a great thing to have.
We will also have to update CI jobs in order to check that PRs don't break the generation of that file.

In long term I would suggest writing the source in typescript, couldn't find any considerations about that, but I don't think there are any drawbacks.

I kinda agree with stof regarding that point, it would probably require a lot of work for not that many benefits due to all the dependencies we work with (and all their supported versions). The only thing we really need is the public API to be typed correctly and recognized by IDEs.

Also there is another small drawback: currently people can use an unreleased version of Encore quite easily by referencing the git repository in their package.json (it can be quite useful, for instance, to ask people if a fix works as intended). Switching to Typescript would add some extra steps to that process and/or require to automate some kind of "experimental" releases on npm.

@TavoNiievez
Copy link

This would make using a webpack.config.ts to configure Encore offer many benefits over the contextual help that the IDE can provide. I think this is a good proposal.

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

No branches or pull requests

4 participants