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: generate valid js code for aliased enum values #1801

Merged
merged 1 commit into from Sep 9, 2022

Conversation

alexander-fenster
Copy link
Contributor

Having an aliased default value in an enum now breaks the generated JS static code:

SyntaxError: Multiple default clauses
    at Espree.raise (/Users/fenster/repos/nodejs-appengine-admin/node_modules/protobufjs-cli/node_modules/espree/dist/espree.cjs:675:25)
    at Espree.raiseRecoverable (/Users/fenster/repos/nodejs-appengine-admin/node_modules/protobufjs-cli/node_modules/espree/dist/espree.cjs:691:18)
    at Espree.pp$8.parseSwitchStatement (/Users/fenster/repos/nodejs-appengine-admin/node_modules/protobufjs-cli/node_modules/acorn/dist/acorn.js:1107:34)
    at Espree.pp$8.parseStatement (/Users/fenster/repos/nodejs-appengine-admin/node_modules/protobufjs-cli/node_modules/acorn/dist/acorn.js:909:39)
    at Espree.pp$8.parseBlock (/Users/fenster/repos/nodejs-appengine-admin/node_modules/protobufjs-cli/node_modules/acorn/dist/acorn.js:1236:23)
    at Espree.pp$5.parseFunctionBody (/Users/fenster/repos/nodejs-appengine-admin/node_modules/protobufjs-cli/node_modules/acorn/dist/acorn.js:3285:24)
    at Espree.pp$8.parseFunction (/Users/fenster/repos/nodejs-appengine-admin/node_modules/protobufjs-cli/node_modules/acorn/dist/acorn.js:1358:10)
    at Espree.pp$8.parseFunctionStatement (/Users/fenster/repos/nodejs-appengine-admin/node_modules/protobufjs-cli/node_modules/acorn/dist/acorn.js:1058:17)
    at Espree.pp$8.parseStatement (/Users/fenster/repos/nodejs-appengine-admin/node_modules/protobufjs-cli/node_modules/acorn/dist/acorn.js:903:19)
    at Espree.pp$8.parseTopLevel (/Users/fenster/repos/nodejs-appengine-admin/node_modules/protobufjs-cli/node_modules/acorn/dist/acorn.js:817:23) {
  index: 1011,
  lineNumber: 54,
  column: 3
}

Fixing that by making sure that the default: clause is emitted only once. Updating the tests accordingly.

@@ -18,18 +18,20 @@ var Enum = require("./enum"),
* @ignore
*/
function genValuePartial_fromObject(gen, field, fieldIndex, prop) {
var defaultAlreadyEmitted = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would usually use let, but it looks like var is internally consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's the remains of Node 4 support. We probably want a global refactor to make the code more modern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @bcoe and @alexander-fenster,

Firstly, Thanks for developing and maintaining this amazing library.

Second, I use protobuf.js massively in IoT embedded devices without experiencing any issues.
Due to small memory footprint I use embedded Javascript engine - Duktape (ES5/5.1 complaint + some more features from ES6/7).

If it is planned to use modern syntax for feature/bug fixes, I am concerned about Duktape compatibility issues with the new code. In addition, I don't believe there are better protobuf alternatives in Javascript.

In the future, I see 2 options:


  1. I maintain a forked ES5/5.1 version equivalent to protobuf.js releases (which could be a nightmare)
  2. You’d also be maintaining another branch for ES5/5.1 syntax.

Requesting your thoughts and guidance on the same.

Thanks!

@alexander-fenster alexander-fenster merged commit 7120e93 into master Sep 9, 2022
@alexander-fenster alexander-fenster deleted the allow-alias branch September 9, 2022 20:15
@github-actions github-actions bot mentioned this pull request Sep 9, 2022
jtbandes added a commit to foxglove/studio that referenced this pull request Oct 18, 2022
**User-Facing Changes**
Fixed an issue where certain Protobuf enum values would fail to load
with a SyntaxError.

**Description**
Fix was in protobufjs/protobuf.js#1801. Added a
unit test to ensure parsing succeeds.

Example file containing dummy enum that reproduces the issue before this
fix:
[dupe_enum.mcap.zip](https://github.com/foxglove/studio/files/9814444/dupe_enum.mcap.zip)
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