Skip to content

Bugfix: Prefix generated static variables to avoid reserved keyword clashing#559

Closed
mcos wants to merge 1 commit intoprotobufjs:masterfrom
mcos:bugfix/static-variable-keyword-clashing
Closed

Bugfix: Prefix generated static variables to avoid reserved keyword clashing#559
mcos wants to merge 1 commit intoprotobufjs:masterfrom
mcos:bugfix/static-variable-keyword-clashing

Conversation

@mcos
Copy link
Copy Markdown

@mcos mcos commented Dec 15, 2016

I noticed earlier while playing around with this library that if a protobuf file namespace has reserved keywords, then a variables are generated with reserved keyword names. Importing the file then causes errors.

syntax = 'proto3';

package var.for.switch;

message Sampler {
	string sample = 1;
}

Compiling this and requiring it gives the following:

var var = {};
            ^^^
SyntaxError: Unexpected token var

To get around this, I simply prefixed the generated variable name with $, which seemed to be in keeping with the conventions of this library. These variables are generated within closures, so they are not assigned anywhere that they may be accessed by a consumer of the generated code.

I understand that this fix may be over-simplified, so I'm happy to work on a better solution than this. I also understand that there are no tests for this module yet, but if some were added, I'd be happy to contribute a test case for this.

@dcodeIO
Copy link
Copy Markdown
Member

dcodeIO commented Dec 15, 2016

There are a few issues with this:

  • While the declaration is renamed, subsequent uses are not (i.e. when returning from the closure or assigning properties).
  • If a namespace is named protobuf, it will hide the global $protobuf var for example.

@dcodeIO
Copy link
Copy Markdown
Member

dcodeIO commented Dec 15, 2016

Does this fix it?

@mcos
Copy link
Copy Markdown
Author

mcos commented Dec 15, 2016

Ah yeah, that was hacky of me, and I see now that $protobuf would be shadowed, which is really not good. Thanks for the fixes in be3e0d9 and 9b7b92a!

@mcos mcos closed this Dec 15, 2016
@mcos mcos deleted the bugfix/static-variable-keyword-clashing branch January 10, 2017 18:05
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