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

TS type imports broken in v1.0.1 #5704

Closed
ktiy opened this issue May 21, 2020 · 16 comments · Fixed by #5733
Closed

TS type imports broken in v1.0.1 #5704

ktiy opened this issue May 21, 2020 · 16 comments · Fixed by #5733
Labels
bug Something isn't working correctly cli related to cli/ dir

Comments

@ktiy
Copy link

ktiy commented May 21, 2020

im no good at opening bug reports but here;
this kind of stuff was working pre-1.0.1, but it isn't working now
image
not entirely sure what the issue is so i apologize that this isn't the most informative issue request, as well as not-so-good title

@bartlomieju
Copy link
Member

@FOX-cat please provide some source code for reference

@garronej
Copy link
Contributor

Hi @bartlomieju,
@FOX-cat errors come from EVT, the module has indeed been broken by v1.0.1.
Investigating the issue. I will report what I found here.

Regards,

@garronej
Copy link
Contributor

garronej commented May 21, 2020

@bartlomieju #5029 is the commit that breaks EVT.
I think the problems lies in the circular type imports but I am having a hard time isolating the issue as I get so many compile error with Deno but I get no error when compiling for myself with tsc...

@ry
Copy link
Member

ry commented May 21, 2020

@garronej @FOX-cat Do you have a way to reproduce the issue? It looks like a typescript error - which is quite odd - there weren't any changes to the type declarations (AFAICT)

@bartlomieju
Copy link
Member

I identified the issue - declaration files are mistakenly analyzed for imports which causes to download referenced libs. I'll have a fix soon.

@garronej
Copy link
Contributor

@bartlomieju @ry thank you for your reactivity!

@bartlomieju
Copy link
Member

@garronej I think I will need some repro after all; the issue I found might not be related to this one.

@garronej
Copy link
Contributor

garronej commented May 21, 2020

Ok, brb
Thanks!

@garronej
Copy link
Contributor

git clone --single-branch --branch 1.7.0 https://github.com/garronej/evt
deno run evt/lib/Ctx.ts

With 1.0.0 it compile, with 1.0.1 it doesn't.
Is it okay?

@bartlomieju
Copy link
Member

@garronej thanks, I will investigate 👍

@garronej
Copy link
Contributor

@bartlomieju No thank you!

@bartlomieju
Copy link
Member

type EvtLike<T> = import("./types/helper/UnpackEvt.ts").EvtLike<T>;
type Evt<T> = import("./types/interfaces/index.ts").Evt<T>;
type CtxLike<T> = import("./types/interfaces/index.ts").CtxLike<T>;
type DoneOrAborted<Result> = import("./types/interfaces/Ctx.ts").DoneOrAborted<Result>;

export type Ctx<Result> = import("./types/interfaces/index.ts").Ctx<Result>;

These lines seem to be the root cause of the problem - dynamic imports that expose types. I guess these were picked by ts.preProcessFile() and somehow analysed.

I tried changing module graph parsing to include dynamic imports but it did not help with this problem. Calling @kitsonk for opinion

@bartlomieju bartlomieju changed the title v1.0.1; breaking changes TS type imports broken in v1.0.1 May 21, 2020
@bartlomieju bartlomieju added bug Something isn't working correctly cli related to cli/ dir labels May 21, 2020
@kitsonk
Copy link
Contributor

kitsonk commented May 21, 2020

I tried changing module graph parsing to include dynamic imports but it did not help with this problem.

We should include dynamic imports in the sources... that still might be hard to grab all the ones TypeScript can see itself and use... I mean structures like the above I find crazy work at all, but obviously they do (like why does it not require an await 🤷). I thought we might still need an escape hatch where if the TypeScript compiler finds something it needs we didn't identify, we need to go grab it and return in sync to the compiler.

@garronej
Copy link
Contributor

garronej commented May 21, 2020

@bartlomieju You are right 👍👍👍
I fixed the module (v1.7.3) by using the new TS type only import syntax instead of import("...").TheType.
However, these changes mean that EVT drops support for TypeScript from 3.4 (Mar 2019) to 3.7 (Feb 2020 )... Some users will have to upgrade TypeScript to keep using EVT in their Node project.
However, thank you for finding the issue 😊
I hope it will get fixed in the next Deno release.

@garronej
Copy link
Contributor

And @bartlomieju @kitsonk, I think maybe you are confusing dynamic import with type import that are two different things although the syntax is the same...

@kitsonk
Copy link
Contributor

kitsonk commented May 22, 2020

@garronej oh yeah, I had totally forgotten about that! Either way, we should support it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly cli related to cli/ dir
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants