fix: Private name "#errorHandler" must be declared in an enclosing class

[TiBianMod]

This PR fixes #3671

on hono-base.ts the route method accepts app: Hono<SubEnv, SubSchema, SubBasePath>, so when using the app instance we don't have access on private methods.

before the errorHandler propery was private errorHandler (only valid on typescript) but now is #errorHandler, so is not accessible anymore.

related: #3596

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.69%. Comparing base (082862b) to head (c02565d).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3673      +/-   ##
==========================================
- Coverage   91.70%   91.69%   -0.01%     
==========================================
  Files         159      159              
  Lines       10145    10149       +4     
  Branches     2851     2879      +28     
==========================================
+ Hits         9303     9306       +3     
- Misses        840      841       +1     
  Partials        2        2              

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

[yusukebe]

@TiBianMod Thanks!

on hono-base.ts the route method accepts app: Hono<SubEnv, SubSchema, SubBasePath>, so when using the app instance we don't have access on private methods.

Does it throw an error? If so, can you share the minimal code to reproduce it?

[TiBianMod]

@yusukebe Thanks,

please checkout https://github.com/TiBianMod/3673-reproduction

but like i set, #errorHandler is not accessible from app

[yusukebe]

@TiBianMod

Thanks. But can you share the code without an external library?

[yusukebe]

@TiBianMod

As you said, app.#errorHandler should not be accessible. But the tests passed and did not throw errors. It's weird.

[usualoma]

From the document, there is no problem with the current code, as the JavaScript specification allows access to private properties of other instances of the same class within the class.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_properties

[TiBianMod]

@yusukebe yes is weird, (i tried some tests but no luck)

for me you can merge this PR (is totally safe and valid)
or we can make the property again private errorHandler: ErrorHandler = errorHandler that is valid only on typescript.

[TiBianMod]

From the document, there is no problem with the current code, as the JavaScript specification allows access to private properties of other instances of the same class within the class.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_properties

@yusukebe
this is exactly the issue we have !!

try this with node

index.js

function errorHandler() {}


class Hono {
    #errorHandler = errorHandler;
}

const app = new Hono();

function route(app) {
    if (app.#errorHandler === errorHandler) {
        //
    }
}

route(app);

[usualoma]

Hi @TiBianMod. Thank you for the explanation.

The errorHandler property was private by definition even before the change in #3596, so the code you showed us at #3673 (comment) would result in an error, which is the expected result of the specification.

However, before #3596, the transpiler was exporting errorHandler as a public property in dist/hono-base.js, so if you weren't doing type checking in TypeScript, the error wouldn't have been detected and it wouldn't have been an error at runtime either.

Given this situation, although #3596 contains a change that is effectively incompatible, errorHandler has always been a private property in Hono, so I think it would be appropriate to fix the application side.

[yusukebe]

@usualoma Thanks for the information!

@TiBianMod Can we close the issue and this PR?

[TiBianMod]

@TiBianMod Can we close the issue and this PR?

sure

[felixmosh]

The following code is wrong... (it is located in hono-base.js)

When there is a usage of the following pattern,

const app = new Hono();
const innerApp = new Hono();
app.route('/', innerApp);

it will fail, since the code in the image is accessing a private member of the "passed" app (which in our example it is innerApp) therefore, the runtime will throw an error.

[yusukebe]

@felixmosh

All our tests pass. Please share your test codes that fail.

[yusukebe]

If '#' is bad, we can implement using 'private'.

[EdamAme-x]

We suppose you could use private in the code and prefix it at build time for readability and all that (#3673 (comment)), but that might be overkill.

[felixmosh]

The code above throws an error since it access private property of a given app param.
So there is some code that tries to access it, therefore, it is not private.

Private means that only the class itself uses it...

[felixmosh]

@yusukebe reproduction repo
https://github.com/felixmosh/hono-private-bug

It is related to the fact that the inner Hono app is exported with commonjs style

[TiBianMod]

@yusukebe can I reopen the PR?

[yusukebe]

@TiBianMod I have ideas. Please wait a bit!