Closed ashleygwilliams closed 6 years ago
As shared in https://github.com/ashleygwilliams/wasm-pack/pull/109#issuecomment-384464904 I vote for generate by default, opt out via cmd args when invoke wasm-pack. Lot of tool understands type definition and provides additional info in js code even, so it benefit js/ts users both.
I vote for yes as well: there is no downside to generating .d.ts
my one thought is pkg size, if you aren't gonna use em, do you want them there? i don't actually know entirely how the npm ecosystem usually deals with this... i might write a follower to get a sense of how many popular packages give a types field.
for pkg size -
node_modules
) will be increased, yes, but mostly really small size. due to nature of type definition, say i.e you have javascript fn
function doSomething(args) {
//really long long long implementation
}
it'd corresponding type definition simply becomes single-line
function doSomething(args: any): any;
Course if it have some interfaces, or enums / dependent types it'll be increased more, but it's plain text files notably small size.
below is one of real-world comparision between impl to type definition from codebase I mostly work with -
actual impl https://cdn.jsdelivr.net/npm/rxjs@6.0.0/internal/operators/audit.js TypeDef https://cdn.jsdelivr.net/npm/rxjs@6.0.0/internal/operators/audit.d.ts
you can see differences.
d.ts
shouldn't be matter as actual js bundle never includes any type definitions.For npm pkgs deal with types - if module author intends to support types by default, it is common to ship type definition along with packages. Most cases it depends on author's preferences, between either 1. would like to support types (either flow, or ts) 2. or doesn't have preferences for it and let 3rd party deals with it. Size usually isn't concern compare to actual codebase size which is real concern in production.
@kwonoj the point about bundlers is a good one. ok. i'm coming around to this. perhaps we can land #109 without a flag... curious about what @yoshuawuyts @jamiebuilds might think
The official TypeScript recommendation is to publish .d.ts
along with .js
inside of the same npm package (assuming your package was designed for TypeScript in the first place).
The other alternative is the @types
organization, which is intended more for publishing .d.ts
for third-party packages which are pure-JS (and not intended as a TypeScript package by the author). It's a massive pain to use, though: you have to make a pull request, get it accepted, etc.
FWIW, I totally subscribe @kwonoj 's point of view. I can only see benefits for the consumers of the future packages (no matter they use js or ts).
In my opinion, as an author you should try to provide the widest support possible, and as a heavy TypeScript user, I really, really appreciate when packages are delivered with a synchronized d.ts
file.
As @Pauan already said, it's a hard and painful process to have to depend on 3rd party typings that usually don't follow the same rythm in terms of updates and, hence, tend to be utterly unsynchronized.
ok so, having slept on this here's the deciding question i think:
is there any case in which a crate written in rust and then compiled with wasm-bindgen would NOT benefit from a types file?
@Pauan wrote "assuming your package was designed for TypeScript in the first place" <-- most people don't design a package for type script, tho writing a package with Rust and compiling to wasm might take the need to design for type script out of the question?
assuming there is ZERO effort required on the authors part to potentially have the types be useful for the package, let's do it by default. otherwise let's not. does that make sense?
@alexcrichton i'm curious- why did you make the typescript definitions opt in? we're discussing making them the default here and i think it'd be a little weird for bindgen and pack to have different defaults...
compiled with wasm-bindgen would NOT benefit from a types file?
The only case I can currently think of is if types generated from bindgen does not corretly match to signature of original rust fn for some reason, gives incorrect signature to consumers.
@ashleygwilliams some good points! I think it's actually a historical accident that wasm-bindgen
doesn't emit .d.ts files by default. Originally it only emitted typescript and then got an option to emit JS instead. Later I realied that .d.ts existed and wanted to switch to JS by default so I swapped the two. I don't think I ever stopped to realize that *.d.ts is harmless and would be good to generate by default!
tl;dr; I'd be totally fine generating *.d.ts by default, and I agree wasm-pack and wasm-bindgen shoud have the same defaults
I've opened https://github.com/rustwasm/wasm-bindgen/pull/173 to change wasm-bindgen's default, which can probably land in tandem with the corresponding wasm-pack PR
@ashleygwilliams most people don't design a package for type script, tho writing a package with Rust and compiling to wasm might take the need to design for type script out of the question?
When I said "designed for TypeScript" I meant "either written in TypeScript, or written in a compile-to-JS language which has support for automatically emitting .d.ts
files". In other words, anything which doesn't require the programmer to manually write a .d.ts
file. Obviously wasm-bindgen qualifies for that.
@ashleygwilliams is there any case in which a crate written in rust and then compiled with wasm-bindgen would NOT benefit from a types file?
If an ES6 project imports the compiled Rust crate, and it doesn't make use of the TypeScript types at all, then obviously it won't benefit. However, it also doesn't hurt anything either (other than the small increase in the npm package size): the bundled ES6 code won't contain any of the .d.ts
stuff.
On the other hand, if we don't include .d.ts
, then anything which makes use of TypeScript types (including TypeScript projects which import the compiled Rust crate), will have a very rough time, since they won't get any static types at all.
In fact it's even worse than that: they will get a compiler error when they try to import the Rust crate:
error TS7016: Could not find a declaration file for module 'foo'. implicitly has an 'any' type.
Try `npm install @types/foo` if it exists or add a new declaration (.d.ts) file containing `declare module 'foo';`
So if we don't include .d.ts
, then we're essentially saying that we don't support importing the compiled Rust crate inside TypeScript projects.
thanks for the added context @Pauan - im' pretty sold on doing this, especially after chatting with @alexcrichton about wasm-bindgen's defaults. i think we'll move forward with this as default behavior assuming the bindgen ends up doing the same.
It did end up happening in https://github.com/rustwasm/wasm-bindgen/pull/173. I think that means we can land @kwonoj's #109 with a no-typescript
flag instead
Could we make it generate both TypeScript and Flow definition files? They are very similar definition formats and it would work basically the same way with a .js.flow
file next to the .js
file
@jamiebuilds this would require wasm-bindgen to support them first at least. Otherwise wasm-pack can't even generate them! I think that's up to @alexcrichton and whether he wants wasm-bindgen to handle that
@jamiebuilds sounds plausible to me! Want to open an issue at wasm-bindgen?
What about using something like flowgen? The actual conversion could be done in wasm-pack, while the CI in wasm-bindgen checks that the emitted typescript is understood by flowgen.
Late by a day, but still few thoughts on this. It is better to have it behind a flag.
But I completely understand about the flexibility and adding a file or two won't hurt the end users (may be some don't like it) But Providing by default support for one type system will not look good on the other type system users. Providing both will have an impact for the people who are using it and for us to maintain it. I would suggest to have them as the pluggable thingy and users can use them as and when needed.
@sendilkumarn There's only two major JS type systems: TypeScript and Flow. And TypeScript is by far the most common, it is the de-facto standard.
And if the cost of maintaining flow types is simply running flowgen, that doesn't sound like a big maintenance burden.
Also, if wasm-pack provides the feature at all (even behind a flag), it has the same maintenance burden, since wasm-pack has to make sure that it is correct and supported (because the user might use the flag).
As for using them "when needed", we're talking about creating libraries, and it is not the library author that needs the types, it is the library consumer. And obviously the library author cannot predict whether all possible library consumers will need the types or not.
if wasm-pack provides the feature at all (even behind a flag), it has the same maintenance burden,
Yeah absolutely agree to this. But for the flag I (personally) feel it is okay to be opinionated and support them and choose not to support others.
that needs the types, it is the library consumer
AFAICT when your library grows and even as a library author you might need type system.
And obviously the library author cannot predict whether all possible library consumers will need the types or not.
Exactly that is what is running on my mind.
But I think it is a thin line here. There is no second thought about the values that type system provides, (in fact I have used them both and enjoyed their benefits). And also provided that wasm-bindgen
now supports it by default. I think it is good to let it be like that 👍
AFAICT when your library grows and even as a library author you might need type system.
The library author is writing their code in Rust, which is already fully statically typed. What we are talking about is a system which takes your Rust types and automatically translates them into TypeScript types, so that people using your Rust crate as a library will benefit. The types will always be in sync with the Rust types.
Chiming in a day late here, but yeah all in favor of adding TS type files out of the box. Not seeing any downsides and tons (!) of upsides!
hey ya'll! it's decided :) we're gonna add TS type files out of the box and we'll see how to do it for flow as well. thanks!
We should consider this issue closed then. Unless you want to wait for #109 to land
yeah i'm waiting for #109 to land.
One note to a comment made up-thread – simply informational, since shipping types is already a thing that's happening:
If an ES6 project imports the compiled Rust crate, and it doesn't make use of the TypeScript types at all, then obviously it won't benefit.
This is true in a very strict sense, if you're using something like ed
as your editor. Increasingly, however, many editors and IDEs will actually use TS definitions to make the JS experience better when interacting with a package that has type definitions. (Depending on configuration, some editors may also do the same with Flow types; I haven't checked recently.) I know that at last VS Code and the JetBrains IDEs do this; I believe the Vim TS plugin may support doing the same from JS files (but I wouldn't quote me on that!).
@chriskrycho oh that's real neat! :D
solved by #109
a question came up in solving issue #107 on PR #109: should we generate typescript type files by default?
i have requested that the PR land behind a flag but i'm curious to hear the arguments for why it should be default! this is the issue for that convo :)