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

Make `Message.prototype.toObject({defaults: true})` use undefined instead of null? #658

Closed
fnlctrl opened this Issue Jan 24, 2017 · 15 comments

Comments

Projects
None yet
2 participants
@fnlctrl
Contributor

fnlctrl commented Jan 24, 2017

protobuf.js version: 6.5.0

I'm trying to send an object that's converted from a message (via toObject({defaults: true})) back to server, but I got something like:

Uncaught TypeError: Cannot read property 'freq' of null

The messages looks like

message Foo {
   Bar bar = 1;  // sometimes not provided by server
}
message Bar {
  string freq = 1;
}

and the js

// bar is not provided by server
var foo = Foo.decode(...).toObejct({defaults: true})   // {bar: null}

....some time later

var message = Foo.encode(foo)  // Uncaught TypeError: Cannot read property 'freq' of null

It was working perfectly fine before 6.5.0. Seems to be caused by the generated code only checking for undefined but not null.
image

So maybe we can make toObject use undefined instead of null? Or let the generated code check for both null and undefined?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 24, 2017

Have you tried with 6.6.1 already?

@fnlctrl

This comment has been minimized.

Contributor

fnlctrl commented Jan 24, 2017

No.. didn't find relevant info in the release notes, so I thought it wasn't fixed. I'll try asap, thanks!

@fnlctrl fnlctrl closed this Jan 24, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 24, 2017

Might not be fixed in 6.6.1, just asking. Feel free to reopen if it's still an issue!

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

Other: Moved micro modules to lib so they can have their own tests et…
…c.; Fixed: Make sure to check optional inner messages for null when encoding, see #658
@fnlctrl

This comment has been minimized.

Contributor

fnlctrl commented Jan 25, 2017

Just got time to test it, and sadly it wasn't fixed in 6.6.1. It seems 2b12fb7 took care of it, when would it be released?

Also, 6.6.1 seems to be forcing Long.js for me when using the light build (with webpack setup)..
So I came up with this workaround (copying most of the lines from index-light)

import Service from 'protobufjs/src/service';
import Type from 'protobufjs/src/type';
import Namespace from 'protobufjs/src/namespace';
import ReflectionObject from 'protobufjs/src/object';
import Root from 'protobufjs/src/root';
import BufferReader from 'protobufjs/src/reader_buffer';
import Reader from 'protobufjs/src/reader';
Reader._configure(BufferReader);
Namespace._configure(Type, Service);
ReflectionObject._configure(Root);
Namespace._configure(Type, Service);
Root._configure(Type);


// finally
var root = Root.fromJSON(json);
var service = Root.lookup('......');

Should I open a separate issue?

@fnlctrl fnlctrl reopened this Jan 25, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 25, 2017

It seems 2b12fb7 took care of it, when would it be released?

Probably soon™.

6.6.1 seems to be forcing Long.js for me when using the light build (with webpack setup)..

Hmm, that shouldn't happen. There shouldn't be a place anymore where long.js is required explicitly. Instead, it's loaded dynamically. Any ideas?

@fnlctrl

This comment has been minimized.

Contributor

fnlctrl commented Jan 26, 2017

image
Webpack's analysis tool shows Long is required by index-minimal (The red dot 135 in the picture).
image

Cause:
https://github.com/dcodeIO/protobuf.js/blob/master/src/index-minimal.js#L48-L58
Seems it's not being inquired

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 26, 2017

Ah, right, the AMD loader. Hrm...

@fnlctrl

This comment has been minimized.

Contributor

fnlctrl commented Jan 26, 2017

Why was Long.js required by default in index-minimal though? I though it was optional and easy-opt-in with protobuf.util.Long = Long

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 26, 2017

Well, it IS meant to be optional. With the inquire thing in place, browserify does not include long by default because it doesn't evaluate AMD loaders at all. That's how it is intended.

Webpack, on the other hand, evaluates the AMD loader shim (in full, light and minimal alike), even though a loader like require.js wouldn't throw if long isn't actually present. You could still exclude it from your project through respective webpack config stuff, or wait until I move the AMD shim out of source to the dist wrapper, which I haven't done yet because browserify doesn't offer a way to specify a custom prelude.

@fnlctrl

This comment has been minimized.

Contributor

fnlctrl commented Jan 26, 2017

Well, I though if it's optional then it may be all right to just not include it at all, and put a notice in the docs about using Long (should be as easy as adding protobuf.util.Long = require('long') after var require('protobuf') if i'm not mistaken?)

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 26, 2017

It's not that easy actually because there is some reconfiguration taking place when Long is installed (perf reasons).

@fnlctrl

This comment has been minimized.

Contributor

fnlctrl commented Jan 26, 2017

Ah, right... missed that part. Maybe Reader can expose a method to use Long explicitly then? Something like protobuf.Reader.useLong(require('long')) where useLong takes care of the reconfiguration?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 26, 2017

I just figured out how to use a custom browserify prelude. Once that's in place, we can remove the define call from source. useLong is a good idea, too, just want to make sure that this happens automatically with AMD.

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

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 26, 2017

For reference: prelude

As you see there, long.js can also be installed manually through assigning it first and then calling protobuf.configure().

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 27, 2017

Feel free to reopen if there are still any issues!

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