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

Add Javascript convenience getters/setters. #5449

Closed
wants to merge 1 commit into from

Conversation

twavv
Copy link

@twavv twavv commented Dec 11, 2018

This is kind of a proof of concept.

I'm happy to add requisite tests and whatnot though I confess I'm not really sure (as of right now) how/where I would do that. It works for me™. Suggestions/guidance appreciated.

Would fix protocolbuffers/protobuf-javascript#93.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@twavv
Copy link
Author

twavv commented Dec 11, 2018

Je l'ai signé!

@googlebot
Copy link

CLAs look good, thanks!

@twavv
Copy link
Author

twavv commented Dec 11, 2018

Just for motivation, this would enable ES2015-idiomatic use of protobuf and reduce a lot of boilerplate code.

function registerUser(user: UserMessage) {
  const {email, password, name} = user;
  db.insert("users", {email, password, name, createdAt: Date.now()});
}

Might also be a way to address protocolbuffers/protobuf-javascript#81 without breaking backwards compatability (by having the getters return default values) but I'm not sure if that's something we want to do.

EDIT: Actually I don't think this should be used to address protocolbuffers/protobuf-javascript#81 (rather have an option on the CLI/in the GeneratorOptions struct).

@BSBandme
Copy link
Contributor

@TeBoring Is this one of what we need for being compatible with react?

And do we need IE8 supported?

@TeBoring TeBoring self-assigned this Dec 14, 2018
@TeBoring TeBoring self-requested a review December 14, 2018 19:08
@TeBoring
Copy link
Contributor

Thanks @travigd. This is really useful. I'll do some experiment internally. See whether it works.

@alexciesielski-pioner
Copy link

This looks great, looking forward to getting this merged.

@shaxbee
Copy link

shaxbee commented Dec 21, 2018

This would resolve the problem that was root cause for creating #5397 as well would enable to simplify typings in grpc-web significantly.

@TeBoring
Copy link
Contributor

TeBoring commented Feb 6, 2019

The new code has some issue with our internal toolchain. Still working on that.

@aldobrynin
Copy link

aldobrynin commented Mar 7, 2019

Any update on this, guys?

@twavv
Copy link
Author

twavv commented Mar 22, 2019

Hi, @TeBoring, any updates? Anything you need me to do? We could also put this behind some kind of feature/CLI flag if we're worried about incompatibilities, yeah?

@twavv
Copy link
Author

twavv commented Feb 1, 2020

Bump! It's been over a year since this has been opened...

@jackielii
Copy link

jackielii commented Feb 1, 2020 via email

@Srinjoy-Santra
Copy link

Can someone please help me with this issue? I think the question in the link is somewhat related to this.
https://stackoverflow.com/questions/60430081/how-to-convert-json-to-proto-message-format-in-javascript-client-side

ctoomey added a commit to livongo/protobuf that referenced this pull request Mar 11, 2020
@jmzwcn
Copy link

jmzwcn commented Jun 1, 2020

@TeBoring,

Could you please let us know what happen to this PR?

@ssilve1989
Copy link

bump?

@jmzwcn
Copy link

jmzwcn commented Apr 23, 2021

any progress?

@twavv
Copy link
Author

twavv commented Apr 23, 2021

If someone else wants to spearhead this, feel free.

@twavv twavv closed this Apr 23, 2021
@jmzwcn
Copy link

jmzwcn commented Sep 6, 2021

Let it is open? at least still have a hope to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setter/Getter Property Access