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

feat: route() with one argument is renamed basePath(). #964

Merged
merged 5 commits into from
Mar 9, 2023

Conversation

usualoma
Copy link
Member

@usualoma usualoma commented Mar 7, 2023

#948 basePath() version

One difference from #948 (comment) is that we replaced private _basePath with #basePath. This should make it more straightforward that it is a local member.

Compatibility

To reduce confusion caused by compatibility issues, it would be possible to allow route(path) to be used in 3.x and then remove it in 4.x.

diff --git a/src/hono.ts b/src/hono.ts
index 72380c8..3238c35 100644
--- a/src/hono.ts
+++ b/src/hono.ts
@@ -137,9 +137,23 @@ export class Hono<
   route<SubPath extends string, SubEnv extends Env, SubSchema>(
     path: SubPath,
     app: Hono<SubEnv, SubSchema>
+  ): Hono<E, RemoveBlankRecord<MergeSchemaPath<SubSchema, SubPath> | S>, BasePath>
+  /** @deprecated
+   * Use `basePath` instead of `route` with one argument.
+   * The `route` with one argument has been removed in v4.
+   */
+  route<SubPath extends string>(path: SubPath): Hono<E, RemoveBlankRecord<S>, BasePath>
+  route<SubPath extends string, SubEnv extends Env, SubSchema>(
+    path: SubPath,
+    app?: Hono<SubEnv, SubSchema>
   ): Hono<E, RemoveBlankRecord<MergeSchemaPath<SubSchema, SubPath> | S>, BasePath> {
     const subApp = this.basePath(path)
 
+    if (!app) {
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      return subApp as any
+    }
+
     app.routes.map((r) => {
       const handler =
         app.errorHandler === errorHandler

image

@usualoma
Copy link
Member Author

usualoma commented Mar 7, 2023

Hi @yusukebe !

I have created a PR that adds basePath(). How about this?

@yusukebe
Copy link
Member

yusukebe commented Mar 8, 2023

@usualoma

Great work!

You're right, I think we should leave route(path) and add a @deprecated comment for compatibility. Let's remove it when we ship the v4.

One thing about #basePath:

I have not used # so far because it is slower than private. If I run the following script in Node.js, it is true that # is slower.

import { run, bench } from 'mitata'

class Sharp {
  #prop = true
  setProp() {
    this.#prop = true
  }
}

class Private {
  private prop = true
  setProp() {
    this.prop = true
  }
}

const withSharp = () => {
  const foo = new Sharp()
  foo.setProp()
}

const withPrivate = () => {
  const foo = new Private()
  foo.setProp()
}

{
  bench('sharp', async () => {
    withSharp()
  })
}

{
  bench('private', async () => {
    withPrivate()
  })
}

await run()

Result:

benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
sharp       66.18 ns/iter  (58.64 ns … 126.23 ns)  66.94 ns 116.58 ns 119.22 ns
private     60.77 ns/iter  (54.91 ns … 129.85 ns)   61.9 ns  73.13 ns 112.74 ns

So I thought it was better not to use #. I don't see any difference when I measure it on Bun, so we may use #. But how about using private _basePath in this time?

@usualoma usualoma force-pushed the feat/basePath-method-2 branch from dd59dcd to b07234b Compare March 8, 2023 20:39
@usualoma
Copy link
Member Author

usualoma commented Mar 8, 2023

@yusukebe Updated!

sharp vs. private

Indeed, with your script, "private" also won in my environment.

benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
sharp       59.18 ns/iter  (53.84 ns … 114.62 ns)  59.99 ns 105.27 ns 107.82 ns
private     56.33 ns/iter  (53.27 ns … 107.28 ns)  57.88 ns  71.64 ns   97.9 ns

However, when I swapped the order of execution as follows ...

import { run, bench } from 'mitata'

class Sharp {
  #prop = true
  setProp() {
    this.#prop = true
  }
}

class Private {
  private prop = true
  setProp() {
    this.prop = true
  }
}

const withSharp = () => {
  const foo = new Sharp()
  foo.setProp()
}

const withPrivate = () => {
  const foo = new Private()
  foo.setProp()
}

{
  bench('private', async () => {
    withPrivate()
  })
}

{
  bench('sharp', async () => {
    withSharp()
  })
}

await run()

The results were also swapped.

benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
private     58.55 ns/iter  (52.36 ns … 116.58 ns)  59.88 ns 103.19 ns 108.03 ns
sharp       55.54 ns/iter  (52.43 ns … 108.97 ns)  57.08 ns  61.97 ns  96.52 ns

I have yet to check all of them, but there is no difference in a modern execution environment. However, I have some misgivings, so I agree with the _basePath at this time. Sorry to add more arguments 🙇

@yusukebe
Copy link
Member

yusukebe commented Mar 9, 2023

The results were also swapped.

Wow, it's right. In my environment also, the results are swapped:

benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
private     65.63 ns/iter  (58.73 ns … 142.83 ns)  66.36 ns 114.72 ns 117.17 ns
sharp       63.15 ns/iter  (56.47 ns … 133.57 ns)  64.38 ns  74.73 ns 115.07 ns

Indeed, I know sometimes the results are swapped in the order of the benchmarks. Thanks for letting me know.

Nevertheless, let's deal with # matter another time. If there is no difference between private and #, we can refactor it to change everything to #.

@yusukebe
Copy link
Member

yusukebe commented Mar 9, 2023

Okay, I'll merge it now!

@yusukebe yusukebe merged commit 55baf9d into honojs:next Mar 9, 2023
@usualoma usualoma deleted the feat/basePath-method-2 branch March 9, 2023 01:10
@yusukebe yusukebe mentioned this pull request Mar 9, 2023
15 tasks
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