search

pinojs / sonic-boom

Extremely fast utf8 only stream implementation
MIT License
261 stars 41 forks source link

Fix cb?.() syntax #198

Closed tedyu closed 2 months ago

tedyu commented 3 months ago

This PR tries to fix the following compilation error:

/opt/app/node_modules/sonic-boom/index.js:393
    cb?.()
       ^

SyntaxError: Invalid or unexpected token
    at Object.replacementCompile (/opt/app/node_modules/append-transform/index.js:60:13)
tedyu commented 3 months ago

@mcollina This change is semantically equivalent. It will allow downstream projects to avoid Invalid or unexpected token error.

tedyu commented 3 months ago

Our services depend on sonic-boom via pino. Recently I downgraded pino release in our service due to the Invalid or unexpected token error mentioned in the description.

jsumners commented 3 months ago

How can the error be reproduced? We need that reproduction in our test suite. Otherwise, this is supported syntax on all of the Node.js versions we target.

tedyu commented 2 months ago

Looking at the build error:

/opt/app/node_modules/sonic-boom/index.js:393
    cb?.()
       ^

SyntaxError: Invalid or unexpected token
    at Object.replacementCompile (/opt/app/node_modules/append-transform/index.js:60:13)
    at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
    at /opt/app/node_modules/append-transform/index.js:64:4

It seems append-transform doesn't accept optional chaining.

@jsumners @mcollina Have you seen any user reporting similar issue ?

Thanks

jsumners commented 2 months ago

What is append-transform?

tedyu commented 2 months ago

According to https://www.npmjs.com/package/append-transform

Install a transform to require.extensions that always runs last, even if additional extensions are added later

tedyu commented 2 months ago

@jsumners With the proposed change, users of sonic-boom (along with append-transform) wouldn't need to worry about configuration.

jsumners commented 2 months ago

I'll let @mcollina have the final word. In my view, the reason a reproduction cannot be added to the tests is because there is not a bug here. The newly provided information clearly indicates there is a bug in some other package.

tedyu commented 2 months ago

My coworker has agreed for us to downgrade pino release so that we don't have to spend time on tuning append-transform. My PR would allow us (and other users who meet similar error) to upgrade to newer pino release. Note: our node version is 18.

mcollina commented 2 months ago

Unless a test is added, it's very likely we will regress. Closing for now. Feel free to reopen if you can provide a test.