Add TypeScript declaration maps

remcohaszing commented 1 year ago

Initial checklist

[x] I read the support docs
[x] I read the contributing guide
[x] I agree to follow the code of conduct
[x] I searched issues and couldn’t find anything (or linked relevant results below)
[x] If applicable, I’ve added docs and tests

Description of changes

This enables “Go to Definition” in TypeScript based editors. Without a declaration map, this takes the user to the type definition. With a declaration map, this takes the user to the location in the source code.

The downside is, this means the source code needs to be published to npm as well. For projects using types in JSDoc this is not a problem, as the source code is already published as-is.

I’m creating a PR here to propose it, but I actually thing this is useful for all of unified. It might be too much work going over all packages just to enable it, but I think it’s useful to enable when a project is already being touched.

codecov-commenter commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (fe6e9e8) 100.00% compared to head (263ca06) 100.00%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #230 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 3 3 Lines 1367 1367 ========================================= Hits 1367 1367 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

remcohaszing commented 1 year ago

The CI failure seems unrelated :thinking:

ChristianMurphy commented 1 year ago

An interesting idea @remcohaszing! It there are times when it be nice if it jumped directly to source. A few thoughts on the implications:

would this be unexpected for TypeScript users? Since not may projects include type maps, people may be used to looking through the declaration file.
For the average user, would viewing the source be easier? The declaration file gives a sense of the package without implementation details. Are developers usually looking for "how is the implemented?" (anecdotally, when I jump to definition 9 times out of 10 I'm looking for related interfaces, not how the function is implemented).
Would this increase the size of the package on NPM? How do build tools handle type maps? (would this increase final built application size for web users?)

ChristianMurphy commented 1 year ago

The CI failure seems unrelated

It is unrelated. Though with multiple canaries failing, it may be worth filing issues downstream to look into what is breaking.

wooorm commented 1 year ago

I looked into it a bit ago and there are no downstream issues in the actual repos. I think it‘s a problem while migrating that it’s expected that there are different versions. But the canary script currently removes all versions of, say, unified. or vfile: https://github.com/vfile/vfile/actions/runs/5485339465

wooorm commented 1 year ago

Yah, how big are the *.maps?

Why should packages decide how this functionality works, and not TS, or users?

remcohaszing commented 1 year ago

Those are all good questions!

For convenience I’m going to call a TypeScript based editor VSCode

would this be unexpected for TypeScript users? Since not may projects include type maps, people may be used to looking through the declaration file.

For the average user, would viewing the source be easier? The declaration file gives a sense of the package without implementation details. Are developers usually looking for "how is the implemented?" (anecdotally, when I jump to definition 9 times out of 10 I'm looking for related interfaces, not how the function is implemented).

Why should packages decide how this functionality works, and not TS, or users?

VSCode provides 2 options: Go to Definition and Go to Type Definition.

Go to Type Definition takes the user to the .d.ts file that defines a type or value.
Go to Definition checks if a declaration map exists. If it exists, it tries to take the user to the source file. Otherwise it falls back to the type definition.

Since many packages don’t ship the TypeScript source code nor the declaration maps, TypeScript falls back to the type definitions for those packages. So essentially shipping declaration maps is needed to give users this choice.

A recent Twitter poll shows that ~90% of over 2600 users expect this behaviour.

Would this increase the size of the package on NPM? How do build tools handle type maps? (would this increase final built application size for web users?)

Yah, how big are the *.maps?

This lists the file sizes of the unified lib directory. I was personally expecting something bigger.

$ ls -lh lib
total 104K
-rw-rw-r-- 1 remco remco  194 Aug 30 14:53 callable-instance.d.ts
-rw-rw-r-- 1 remco remco  155 Aug 30 14:53 callable-instance.d.ts.map
-rw-rw-r-- 1 remco remco 1.2K Aug 14 20:38 callable-instance.js
-rw-rw-r-- 1 remco remco  42K Aug 30 14:53 index.d.ts
-rw-rw-r-- 1 remco remco 5.2K Aug 30 14:53 index.d.ts.map
-rw-rw-r-- 1 remco remco  40K Aug 30 14:49 index.js

This doesn’t affect build tooling or bundle size. It only affects the npm package size.

ChristianMurphy commented 1 year ago

A recent Twitter poll shows that ~90% of over 2600 users expect this behaviour.

Good to see some numbers, to be more specific 85.5% at the time of writing. I may be the odd one out, with some C/C++ background, heading files feel more intuitive to me 🤷‍♂️ 😅

This lists the file sizes of the unified lib directory. I was personally expecting something bigger.

Smaller than the implementation or declaration, good to know.

This doesn’t affect build tooling or bundle size. It only affects the npm package size.

Good to know.

I think I'm fine with this. I do have two fuzzier concerns.

it feels a bit off needing to distribute a .map with ESM code, VSCode/TS-language-server should be able to statically infer all of this without extra help.
if this is an unequivocal good for developer experience which has been around since version 2.9 (three major versions ago at the time of writing), why isn't this a default enabled option for TS or commonly enabled by other OSS projects?

remcohaszing commented 1 year ago

Good to see some numbers, to be more specific 85.5% at the time of writing.

I didn’t count the :popcorn: votes. Anyway, it’s a pretty high percentage. :)

it feels a bit off needing to distribute a .map with ESM code, VSCode/TS-language-server should be able to statically infer all of this without extra help.

It’s a source map for the type definitions, which are generated from the source code.

The whole Go to Definition subject is a bit of a hot topic on Twitter currently. TypeScript 4.7 added support for Go to Source Definition. However, this is slow and the author himself isn’t a fan. I won’t pretend I understand the details. See https://github.com/microsoft/TypeScript/issues/49003

if this is an unequivocal good for developer experience which has been around since version 2.9 (three major versions ago at the time of writing), why isn't this a default enabled option for TS

The declaration option isn’t even on by default. That would be a prerequisite.

or commonly enabled by other OSS projects?

From my experience I’ve seen that people can perform absolute magical types that far exceed the types I can come up with. However, most people don’t have deeper understanding of topics such as the meaning of TypeScript options, JSX, or exports. I think this option is simply not well known.

Also this makes the editor point to the source code, not the compiled code. Many packages don’t publish the source code to npm. For unified the source code just happens to be the code used at runtime.

ChristianMurphy commented 1 year ago

The whole Go to Definition subject is a bit of a hot topic on Twitter currently. TypeScript 4.7 added support for Go to Source Definition. However, this is slow and the author himself isn’t a fan. I won’t pretend I understand the details. See https://github.com/microsoft/TypeScript/issues/49003

It sounds like the concerns center around:

there are still dynamic module formats like CJS and UMD which are difficult to impossible to statically analyze in some circumstances
there are hand authored definition files which may not line up with the implementation at all

We shouldn't run into either of these because:

We use ESM which declares exports statically
We (mostly) generate type definitions from the source, and test types heavily

The declaration option isn’t even on by default. That would be a prerequisite.

It could be bundled with declaration, similar to how options like module: node16 automatically sets a bunch of related options.

I think this option is simply not well known.

That's part of my point, it feels like TypeScript itself isn't promoting this feature much. Which makes me wonder if they will further enhance or push alternatives.

wooorm commented 1 year ago

The whole Go to Definition subject is a bit of a hot topic on Twitter currently.

I think this option is simply not well known.

That's part of my point, it feels like TypeScript itself isn't promoting this feature much. Which makes me wonder if they will further enhance or push alternatives.

I think the thing this PR solves is good. But I feel like hot topics are not something we need to push. Sounds like something the opinions on will change, and like something TS will change.

It takes a lot of time to go through packages. It takes time to flip switches on and then later switch them off because they’re a default, or there’s an improvement.

remcohaszing commented 1 year ago

I think the thing this PR solves is good. But I feel like hot topics are not something we need to push. Sounds like something the opinions on will change, and like something TS will change.

I don’t think we should do this because it is a hot topic. It being a hot topic brought it to my attention and I think it’s a good idea.

It takes a lot of time to go through packages. It takes time to flip switches on and then later switch them off because they’re a default, or there’s an improvement.

This is why I propose to enable it when a package is already being touched, but not to actively go over all packages just to enable this.

wooorm commented 1 year ago

This is why I propose to enable it when a package is already being touched, but not to actively go over all packages just to enable this.

It still takes time to undo it, or change it, when TS decides it should be a default, or that it should be a slightly different way, or so.

I don’t think we should do this because it is a hot topic.

I precisely don’t want to do it yet while it is a thing being debated/changed/not good yet.

To summarize, this is for “Go to Definition”, which is new in 4.7, which humans apparently all want, but the author doesn’t like because it’s slow, and the feature isn’t pushed by the TS team.

That doesn’t sound great yet.

remcohaszing commented 1 year ago

To summarize, this is for “Go to Definition”, which is new in 4.7, which humans apparently all want, but the author doesn’t like because it’s slow, and the feature isn’t pushed by the TS team.

You seem to confuse two distinct features here.

This PR is to support “Go to Definition”, a feature introduced in TypeScript 2.9. This feature takes the user to the original source code.
TypeScript introduced “Go to Source Definition”. This feature takes the user to the emitted JavaScript code, but it’s slow.

For a project authored in JavaScript and only emitting types based on JSDoc, these two behave almost the same.

wooorm commented 1 year ago

a feature introduced in TypeScript 2.9

You link to when declaration maps were added. From the text there it reads as if “Go to Definition” already existed, and maps were added after.

The poll you linked asked specifically about .ts vs .d.ts. I wonder how different it would be for .js vs .d.ts.

This side thread is quite interesting: https://twitter.com/atcb/status/1696226086634623431. TS files are already always preferred if they exist.

So: why doesn’t TS do this for JS files?

I think I’m leaning on :+1:ing this though. Particularly due to the last point in the OP, where it recommends using declarationMap: true. I don‘t understand why they don’t recommend it stronger.

github-actions[bot] commented 1 year ago

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.

wooorm commented 1 year ago

Alright, let’s try this out!

unifiedjs / unified