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

"Slow types" feel opinionated #444

Open
paulmillr opened this issue Apr 24, 2024 · 16 comments
Open

"Slow types" feel opinionated #444

paulmillr opened this issue Apr 24, 2024 · 16 comments

Comments

@paulmillr
Copy link

paulmillr commented Apr 24, 2024

JSR sets low score for packages which use type inference:

- export function add(a: number, b: number) {
+ export function add(a: number, b: number): number {
    return a + b;
  }

I've developed quite a few typescript libraries. Never felt a slowdown from inferred types. Moreover, editors like vscode have settings that allow automagically appending inferred types in UI.

Why do we need to rewrite everything now to get proper scores? Because JSR can't be bothered to become fast enough to deduce these types? Feels like a problem that JSR should solve internally, instead of forcing an opinion on users.

@mrlubos
Copy link

mrlubos commented Apr 26, 2024

Same issue #447

@lucacasonato
Copy link
Member

Hey!

JSR uses type information for multiple features, for example to be able to emit type declarations and generate documentation. To do this, we need to look at the types of any exported symbols (functions, classes, public class properties, public class methods, class constructors, const bindings, let bindings, and var bindings).

Usually, looking at type information requires full type checking. Full type checking is broadly four things:

  • tracking type of different symbols (across files)
  • being able to determine equivalence of two types (eg is number a subset of number | string, or is { a: true } a subset of { a: boolean })
  • flow control and type narrowing (ie const x: number | string; if (typeof x !== "string") { const y: number = x; // this is ok }.
  • inferring types (when you do not explicitly set a type, infer a type based on context)

There are a couple of problems with full type checking though. The only tool that implements this right now is the TypeScript compiler (tsc). tsc has a couple of problems for us.

Firstly, tsc changes over time. This means that code you write today results in one version of type information being generated today. But tomorrow, or more realistically a year from now, tsc may have improved or changed it's inference so that the resulting types from the code you wrote are now different (for example more precise). This is a problem for us, because it means that if you upload code today, we can not safely re-emit or re-generate documentation in a year, because the tsc may have changed the semantic meaning of the code to be different to what you intended it to be when you published the code. Additionally tsc may remove a feature of compiler flag so that the code you wrote today can not be checked with future versions of tsc at all.

Secondly, tsc is pretty slow at full type checking and especially inference. If we were to perform full type checking on all uploaded code on each publish, publishing could take well upwards of a minute for some projects. This is because type checking is not just affected by the current package, but also the number and count of dependent packages. This is not acceptable both because it is not a good user experience, but also because this is a lot of CPU time cost that we can not reasonably bear.

Thirdly, as I mentioned earlier, type checking with inference is not just affected by code and configuration the current package, but also by all dependent packages. This is a problem, because dependent packages can change over time. You could publish a new version of a package that changes the types of exported symbols in a way that results in a different inferred type in your local package code. Thus if we check your code with a current dependency version, it may not match a type check we perform in a month with a newer dependency version.

With this in mind, JSR encourages projects to not use complex type inference ("slow types") for any exported symbols. Because this requires that you specify explicit type annotations in the source code, it allows us to get all type information needed for declaration emits and documentation generation purely from the syntax of the code, without having to perform any part of type checking or even resolving of dependencies. No tsc is needed. This ensures that for any uploaded code, we can generate docs or declaration emits at any time in the future, without changing semantic meaning.

This is a direction TypeScript is going separately from JSR via the new --isolatedDeclaration option in TypeScript 5.5. It enforces similar stricter rules around where type annotations are needed in files.

So to conclude: we do not enforce type annotations "just for fun", or because we think it's "just the right thing to do". We do it to overcome technical issues posed by the current state of the TypeScript ecosystem. If the conditions change in the future, the code that JSR allows may change - but for now, explicit type annotations are needed to ensure a stable declaration output and documentation generation.

Happy to answer any other questions you may have :)

@mrlubos
Copy link

mrlubos commented Apr 26, 2024

Gotcha @lucacasonato and thanks for the detailed explanation. With that context in mind, I feel okay with adding explicit types to our generated functions at https://github.com/hey-api/openapi-ts. However, relying on people to tell us that our generated code isn't compliant with JSR won't be ideal. Is there any tooling we can use to make sure our generated code is good to go for people who use it in their packages that are being published to JSR?

@paulmillr
Copy link
Author

Thanks Luca. That makes sense. A few thoughts:

Firstly, tsc changes over time. This means that code you write today results in one version of type information being generated today

JSR can mention "generated with tsc 5.2" (i) "may not be relevant with other versions". And / or re-generate declarations once per X months.

Secondly, tsc is pretty slow
it is not a good user experience, but also because this is a lot of CPU time cost that we can not reasonably bear

Can this process be done client-side? An optional flag like jsr publish --generate-types - would still be better than "slow types". Also most OSS projects are using GitHub, so if users would use GitHub CI, that would be free for them.

As for re-using dependency types - yeah, I don't see how this could be solved for packages which use version ranges. However, for packages which set versions to an exact value, that's easy. Unless dependencies of dependencies also use version ranges.

@lucacasonato
Copy link
Member

@mrlubos Yes, you can use either jsr publish --dry-run, or once ts 5.5 is out, run with --isolatedDeclarations

@paulmillr You may be interested in #15. You could run this before publishing in CI, and in most cases it will be able to fix all the slow types 👍

@paulmillr
Copy link
Author

@lucacasonato proper direction.

@vijay8i
Copy link

vijay8i commented May 24, 2024

I want to chime in as well to say that "slow types" feels opinionated, albeit from a different perspective. As a library developer, I prefer writing JavaScript and document types using JSDoc to get most of the same benefits of writing in TypeScript without a compilation step. Fwiw, Svelte/Kit prefers the same approach for libraries.

My interest in publishing to JSR instead of NPM is to foster ESM more than the use of TypeScript. So having the score reduced because I prefer writing in JavaScript seems a limiting experience and makes me want to reevaluate my choice on where to publish.

@lucacasonato
Copy link
Member

@vijay8i You're score is reduced for not having types, not for writing JavaScript. If you publish dts files for your project, you will get the points related to slow types.

@KnorpelSenf
Copy link

@lucacasonato I am aware of numerous attempts by other people to “just rewrite tsc in Rust” and they all have failed in one way or another. However, the general case you are describing is not the common case. Most functions and symbols exported by libraries have rather simple types that could be inferred easy by some Rust-based analysis, i.e. a tiny subset of tsc that works well for the common case but falls back to reporting slow types in the complex case.

Do you think it is worth writing a tool that can infer some return types in some easy cases, so that JSR can use it with a best-effort approach?

@vijay8i
Copy link

vijay8i commented May 24, 2024

@vijay8i You're score is reduced for not having types, not for writing JavaScript. If you publish dts files for your project, you will get the points related to slow types.

@lucacasonato Thanks. Good to know. Looks like TypeScript can generate .d.ts from .js files. It would improve the DX of publishing to JSR if it could figure out how to do the invocation automatically or through some directive in either jsr.json or package.json. I don't want to have a TSC dependency in my library - when developing or to publish. I realize I am in the minority and might sound like nitpicking. But I do feel that having this feature would bring in a good chunk of developers in the pure JavaScript camp to move to JSR.

Also apologies to @paulmillr, it is not my itention to sidejack the original issue. Happy to open a new issue if its worth creating one.

@lucacasonato
Copy link
Member

@KnorpelSenf It is the aim of slow types to report diagnostics only for cases that can not be analyzed by looking at the source code of the current module.

For example, we can infer: const x = () => "hello". Soon this will extend to some return statements in functions too. If you think there is something that we should be able to analyze, but can't, please open a separate issue.

Also see https://jsr.io/docs/about-slow-types#simple-inference


@vijay8i We can not invoke tsc server side, because it is not a stable piece of software. See this comment: #444 (comment).

If you want to emit .d.ts files from your .js + tsdoc sources, you will have to do this locally using your own tsconfig.json file. We are tracking being able to emit some JS + tsdoc server side without TSC in #494.

@m5nv
Copy link

m5nv commented May 24, 2024

If you want to emit .d.ts files from your .js + tsdoc sources, you will have to do this locally using your own tsconfig.json file. We are tracking being able to emit some JS + tsdoc server side without TSC in #494.

@lucacasonato #494 is fire. Wish I had the Deno chops to get it done. So looking forward to it getting implemented soon(ish?). Thanks!

@elvisvoer
Copy link

@vijay8i You're score is reduced for not having types, not for writing JavaScript. If you publish dts files for your project, you will get the points related to slow types.

May I ask how do you do that? I have tried to find an example in the docs but there is none. I have tried to use publish and in the include section to have the entire dir containing both the js and the .d.ts file and nothing seems to work.

I understand that the docs are stating:

JSR supports and encourages publishing TypeScript source code rather than pairs of .js + .d.ts files.

But I believe there should still be an example of how to do this with the pair of files.

@vijay8i
Copy link

vijay8i commented May 27, 2024

May I ask how do you do that? I have tried to find an example in the docs but there is none. I have tried to use publish and in the include section to have the entire dir containing both the js and the .d.ts file and nothing seems to work.

Same question. I did the following and I still see slow types warning in the score.

  "publish": {
    "include": [
      "README.md",
      "package.json",
      "src/**/*.?s"
    ]
  }

There are a few more annoyances. I don't find it impractical to maintain information in two different files (package.json and jsr.json). IMHO, the information jsr needs should simply be a stanza within package.json. I don't understand the technical reasoning for having to create and maintain jsr.json but it just doesn't feel right.

@marvinhagemeister
Copy link
Contributor

May I ask how do you do that? I have tried to find an example in the docs but there is none. I have tried to use publish and in the include section to have the entire dir containing both the js and the .d.ts file and nothing seems to work.

Right now the .js files need a triple slash reference comment at the top to find the related type file. It's on the roadmap to do the discovery automatically, see #370

@elvisvoer
Copy link

@marvinhagemeister that worked pretty well. Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Triage
Development

No branches or pull requests

8 participants