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

Use const instead of var when outputting es6 descriptor #667

Closed
atrauzzi opened this Issue Jan 30, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@atrauzzi

atrauzzi commented Jan 30, 2017

I've got this in my descriptor/metadata:

var $root = ($protobuf.roots["default"] || ($protobuf.roots["default"] = new $protobuf.Root()))

Which makes my default-configured linter a little sad. Maybe it's worth switching to const or failing that let?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 30, 2017

ES6 wrapper for reference: https://github.com/dcodeIO/protobuf.js/blob/master/cli/wrappers/es6.js

The $root var is generated on the JS side (shared for all sorts of static code), which is then just wrapped with the wrapper referenced above. What you can do today is providing a compatible linter configuration to pbjs with the --lint parameter. If you have another idea that still allows reusing the shared code part, let me know.

@atrauzzi

This comment has been minimized.

atrauzzi commented Jan 30, 2017

Ah, I see.

What about separating the templating process out so that the assignments go to their respective sides, putting const $root = in the ES6 template and var $root = in the ES5 one?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 30, 2017

For reference:

As you can see there, $root definitions are slightly different depending on the type of module you are generating (JSON module use new Root() while static code uses {}). That's why it is in the respective target's code. Hmm...

Also, with static code there are var keywords all over the place. That should be fixed then too, I assume. Could be just an option, i.e. --es6, provided to pbjs or something.

@dcodeIO dcodeIO added the enhancement label Jan 30, 2017

@atrauzzi

This comment has been minimized.

atrauzzi commented Jan 30, 2017

I feel like I'm looking at something that resembles view logic in a web application. What would your thoughts be on pulling in a view engine as a dev dependency and doing lots of this declaratively?

dcodeIO added a commit that referenced this issue Jan 30, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 30, 2017

I'd rather avoid rewriting everything (again). Nonetheless, there is an --es6 option now (automatically enabled when using --wrap es6).

@atrauzzi

This comment has been minimized.

atrauzzi commented Jan 30, 2017

Completely understandable. I'll try it out this evening and if there are any issues, I'll reopen :)

@atrauzzi atrauzzi closed this Jan 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment