stephenh / ts-proto

An idiomatic protobuf generator for TypeScript
Apache License 2.0
2.16k stars 349 forks source link

importNotUsedAsValues: import observable as a type instead of a value #594

Open paralin opened 2 years ago

paralin commented 2 years ago

error TS1371: This import is never used as a value and must use 'import type' because 'importsNotUsedAsValues' is set to 'error'.

import { Observable } from 'rxjs'

In the generated code by ts-proto, sometimes Observable is not used, so it should have "import type."

stephenh commented 2 years ago

@paralin I think you've been in the guts of ts-proto/ts-poet enough that you've probably discovered this, but in theory this can be fixed by just adding a t: prefix to the imp string here:

    const observable = imp('Observable@rxjs');

Note that we got lucky on this one since, as you said, Observable is only used as a type.

A bigger lift is more generally making ts-proto outputs/imports always "100% right"; I think there are two ways of doing it:

1) Just manually having dual const fooType = imp("t:...") / const fooValue = imp("...") values in ts-proto and having the code "just know" which one to use based on the context.

2) Somehow getting ts-poet to pre-parse it's output and auto-convert "not used as a value" imports to type imports.

In theory 2) is the "cutest" in terms of long-term ROI, but it seems like quite a bit of work up-front, and I'd be worried that adding an AST step to the output would slow things done a bit. (...although we do run through prettier; maybe there is a prettier plugin that auto-type-imports? That would be a really q&d win if so.)

Also one could argue that maybe ts-poet shouldn't be in the business of rewriting imports, which means we're back to 1) of ts-proto's codegen itself just using dual consts and knowing which one to use.

...anyway, all that said, I think the Observable fix is easy. :-)

paralin commented 2 years ago

If you call imp('t:Observable@rxjs') and later imp('Observable@rxjs') it will generate only import type { Observable }.

ts-poet will need to be updated to detect if /any/ non-"type" imports to the same thing are called, and if so, drop the "type" from the import.

The WIP pull request already takes this into account by calling imp('Observable@rxjs') in the one and only place where this is relevant (grpc-web). But, the generated code has import type { Obervable } and thus causes errors.

WIP PR: https://github.com/paralin/ts-proto/commit/f83d8be76746ad929fde8c7d9c29a703b579766f