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

Generate TypeScript declaration maps #1412

Merged
merged 2 commits into from
May 2, 2024
Merged

Conversation

steve-taylor
Copy link
Contributor

In IDEs such as Visual Studio Code and WebStorm, when you command-click (or control-click) on an imported identifier from another package (e.g. rx-player), it does one of the following:

  1. In packages without type definitions, the IDE takes you to the identifier's declaration in the relevant (transpiled) JavaScript file.
  2. In packages that contain type definitions, the IDE takes you to the identifier's type definition.
  3. In packages that contain type definitions, declaration maps and the source files that the declaration maps refer to, the IDE takes you to the declaration in the source.

Currently, rx-player is a type 2 package. This pull request makes it type 3.

The trade-off is the package now includes source, making the package larger. The package size (when packed) is

  • 5.29 MB with source
  • 2.77 MB without source

Personally, I don't mind trading off a few MB for being able to navigate directly to the source.

Before this change:
rx-player-before

After this change:
rx-player-after

@peaBerberian
Copy link
Collaborator

Hi,

I really like the idea thanks, I wasn't aware of that declarationMap thing.

Also I don't have much issues with providing source files through the npm registry, as it is beneficial in the case you exposed. Though this is right that it does add multiple megabytes to all applications' node_modules directory which we cannot ignore.

I guess declaration maps is a tsserver feature, and thus also works on all editors supporting the LSP (so the great majority of them?).

@Florent-Bouisset What do you think?

@Florent-Bouisset
Copy link
Collaborator

Hello, thanks for your proposal
What would be the use case ? I believe a library user expects to be redirected to the type definition (to understand how to use the API). Being redirected to the source code may not be desirable for most users.

@steve-taylor
Copy link
Contributor Author

Hello, thanks for your proposal What would be the use case ? I believe a library user expects to be redirected to the type definition (to understand how to use the API). Being redirected to the source code may not be desirable for most users.

I can hover over a function to see it's signature. It's also used in auto-complete. If I navigate to the source, I'll also see its signature. The source is a superset of the type definitions.

As for the use case, I typically only cmd-click into an imported function/class to see its source when I'm trying to figure out what exactly it's doing. Back in the day when there were mostly JavaScript-only packages, I could cmd-click to see transpiled code which, if not minified, was somewhat informative. With the advent of type definitions, I find it frustrating to be directed to the type definitions – information which I already have available to be without navigating.

@Florent-Bouisset
Copy link
Collaborator

OK, I see, I think it's a legitimate use case. I agree with that change
Can you please add /src/parsers/manifest/dash/wasm-parser/target in .npmignore ?
This folder contain Rust build artifact that are not required to be published

@Florent-Bouisset
Copy link
Collaborator

Hello @steve-taylor can you do the slight change on the .gitignore or do you want us to do it?

@peaBerberian peaBerberian added this to the 4.1.0 milestone Apr 10, 2024
@peaBerberian peaBerberian added the Priority: 2 (Medium) This issue or PR has a medium priority. label Apr 10, 2024
@peaBerberian peaBerberian merged commit 3beaaa3 into canalplus:dev May 2, 2024
4 of 6 checks passed
@peaBerberian peaBerberian mentioned this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 2 (Medium) This issue or PR has a medium priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants