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 cb?.() syntax #198

Closed
wants to merge 4 commits into from
Closed

Fix cb?.() syntax #198

wants to merge 4 commits into from

Conversation

tedyu
Copy link

@tedyu tedyu commented Jun 3, 2024

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)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Could you add a test for this? I have not understood why this happens to begin with. All our tests pass.

@tedyu
Copy link
Author

tedyu commented Jun 7, 2024

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

@tedyu
Copy link
Author

tedyu commented Jun 7, 2024

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
Copy link
Member

jsumners commented Jun 7, 2024

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
Copy link
Author

tedyu commented Jul 2, 2024

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
Copy link
Member

jsumners commented Jul 2, 2024

What is append-transform?

@tedyu
Copy link
Author

tedyu commented Jul 2, 2024

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
Copy link
Author

tedyu commented Jul 2, 2024

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

@jsumners
Copy link
Member

jsumners commented Jul 3, 2024

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
Copy link
Author

tedyu commented Jul 3, 2024

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
Copy link
Member

mcollina commented Jul 3, 2024

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.

@mcollina mcollina closed this Jul 3, 2024
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.

None yet

3 participants