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

fix(types): rm ExcludeEmptyObject to fix massively increased type instantiations #3507

Merged
merged 2 commits into from
Oct 14, 2024

Conversation

m-shaka
Copy link
Contributor

@m-shaka m-shaka commented Oct 11, 2024

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code
  • Add TSDoc/JSDoc to document the code

It's super weird, but if ExcludeEmptyObject is outside of hono-base.ts, "Type instantiation is excessively deep and possibly infinite." error occurs.
Please use this script to check
#3443 (comment)

I don't know the cause at all, it may be a kind of cache issue, though

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.58%. Comparing base (f1a7267) to head (d8600e3).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #3507    +/-   ##
========================================
  Coverage   95.58%   95.58%            
========================================
  Files         155      155            
  Lines        9357     9357            
  Branches     2873     2761   -112     
========================================
  Hits         8944     8944            
  Misses        413      413            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@m-shaka m-shaka force-pushed the fix-increased-instanciations branch from c7e2964 to d8600e3 Compare October 11, 2024 11:47
@m-shaka m-shaka changed the title fix(types): mv ExcludeEmptyObject to hono-base.ts to fix massively increased type instantiations fix(types): rm ExcludeEmptyObject to fix massively increased type instantiations Oct 11, 2024
@@ -159,7 +160,7 @@ type PathToChain<
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type Client<T> = T extends Hono<any, infer S, any>
export type Client<T> = T extends HonoBase<any, infer S, any>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird again.

The reason why I added ExcludeEmptyObject is that schema type with empty object type broke RPC type.
For example, T in code below is never without ExcludeEmptyObject.

const app = new Hono().route('/', new Hono().get('/foo', c => c.json({ ok: true })))
type T = Client<typeof app>

But, replacing Hono with HonoBase in Client fixed the problem

Copy link
Contributor Author

@m-shaka m-shaka Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

const app = new Hono()
  .route('/', new Hono().get('/foo', c => c.json({ ok: true })))
type Extract1<T> = T extends Hono<any, infer S, any> ? S : never
type Extract2<T> = T extends HonoBase<any, infer S, any> ? S : never
type R1 = Extract1<typeof app> // {}
type R2 = Simplify<Extract2<typeof app>> // {} | { '/foo': ... }

typeof app is HonoBase, but I don't suppose it explains this behaviour

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m-shaka

Is that behavior related to the following weird stuff?:

CleanShot 2024-10-14 at 18 35 10@2x

CleanShot 2024-10-14 at 18 36 12@2x

Copy link
Contributor Author

@m-shaka m-shaka Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a matter of display.
The return type of route is HonoBase and it's correct, but in src/hono-base.ts its name is Hono. If you rename class Hono with HonoBase in src/hono-base.ts, you see always HonoBase in the widget.
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m-shaka

Ah, understand. Thanks!

@yusukebe
Copy link
Member

yusukebe commented Oct 14, 2024

Hi @m-shaka

I can reproduce it. Indeed, it's weird.

It's super weird, but if ExcludeEmptyObject is outside of hono-base.ts, "Type instantiation is excessively deep and possibly infinite." error occurs.

How about putting ExcludeEmptyObject in hono-base.ts instead of utils/types.ts (it was my suggestion to move it to utils/types.ts!)?

@m-shaka
Copy link
Contributor Author

m-shaka commented Oct 14, 2024

We don't need ExcludeEmptyObject anymore thanks to https://github.com/honojs/hono/pull/3507/files#diff-786ee0c27ee2360ff838e12e8de257697060cc7bddcb8cb461c8ba1faa664aceR163 and also it will reduce the number of type instantiations more.

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yusukebe
Copy link
Member

@m-shaka

Okay! Let's go with it. Thank you.

@yusukebe yusukebe merged commit 3311664 into honojs:main Oct 14, 2024
16 checks passed
TinsFox pushed a commit to TinsFox/hono that referenced this pull request Oct 28, 2024
…tantiations (honojs#3507)

* fix: mv ExcludeEmptyObject to hono-base.ts

* refactor: rm ExcludeEmptyObject
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 this pull request may close these issues.

2 participants