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

bug: type only files are not emitted #6746

Closed
lucacasonato opened this issue Jul 14, 2020 · 8 comments · Fixed by #6760
Closed

bug: type only files are not emitted #6746

lucacasonato opened this issue Jul 14, 2020 · 8 comments · Fixed by #6760
Assignees

Comments

@lucacasonato
Copy link
Member

lucacasonato commented Jul 14, 2020

❯❯❯ cat types.d.ts 
export interface HelloWorld {}
❯❯❯ cat mod.ts 
export * from "./types.d.ts";
❯❯❯ deno info mod.ts
error: Failed to get compiled source code of file:///mnt/f9/Projects/github.com/denoland/deno_registry2/types.d.ts.
Reason: No such file or directory (os error 2)

Try it out locally: $ deno cache https://git.io/JJsqz

The quick hacky fix is to forcefully emit an empty file when when tsc skips emitting the file. We know which files we are expecting to be emitted because they should map 1:1 to the input (source) files.

cc @bartlomieju

@bartlomieju bartlomieju self-assigned this Jul 14, 2020
@kitsonk
Copy link
Contributor

kitsonk commented Jul 15, 2020

There is no valid emit for .d.ts files, because they don't contain runtime code. Why are you expect an emit?

Instead of "hacking around it" deno info should be able to determine that some source files (.d.ts) don't have a compiled version.

@bartlomieju
Copy link
Member

@kitsonk certainly there's nothing to emit as it's .d.ts file - but why does TS compiler doesn't elide that import then? I guess this is closely related to the problem you faced with --no-check option, but I don't think adding importsNotUsedAsValues: error as default setting will solve this issue for compilation (because it'd be a major breaking change). Then how could we handle such situation? I thought about returning empty string when .d.ts file is requested by V8.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 15, 2020

but why does TS compiler doesn't elide that import then?

Because TypeScript doesn't know that at runtime that doesn't resolve to some JS module that has a runtime it... it thinks of .d.ts could be "shadow" of JS files. If the export was named exports of interfaces, it would get elided. Another reason to use export type, because sometimes it cannot be statically determined.

@bartlomieju
Copy link
Member

but why does TS compiler doesn't elide that import then?

Because TypeScript doesn't know that at runtime that doesn't resolve to some JS module that has a runtime it... it thinks of .d.ts could be "shadow" of JS files. If the export was named exports of interfaces, it would get elided. Another reason to use export type, because sometimes it cannot be statically determined.

Granted and I completely agree.

That still leaves a question, how do we handle this situation gracefully?

Would returning empty file contents to V8 work?

We can't just leave this situation as is, because it's happening to many people (even though it's "their" fault for not using import type/export type):

@bartlomieju
Copy link
Member

So I did tentative fix for this issue in #6760 by providing empty module for declaration files. I don't think that's the best solution as it relies on .d.ts file extension which is not guaranteed especially for remote modules. Any ideas welcome

@kitsonk
Copy link
Contributor

kitsonk commented Jul 15, 2020

What about just returning any empty file for a compiled cache miss?

@bartlomieju
Copy link
Member

What about just returning any empty file for a compiled cache miss?

@kitsonk I guess that would also work, I need to think if there are unwanted side effects of this approach... 🤔

@kitsonk
Copy link
Contributor

kitsonk commented Jul 15, 2020

Less panics? Covering over some other internal defect? We could log a warning saying that an empty module was detected, and that the user is likely doing something wrong. That way any issues wouldn't get swallowed, but they wouldn't result in panics.

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

Successfully merging a pull request may close this issue.

3 participants