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

Internals change breaks pre-generated file #649

Closed
jkrems opened this Issue Jan 17, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@jkrems

jkrems commented Jan 17, 2017

protobuf.js version: protobufjs@6.5.0

The following code has been generated by a previous version (I think 6.4.5):

                        RecordSet.from = function from(source, options) {
                            return this.convert(source, $protobuf.converters.message, options);
                        };

Running with the latest version of protobufjs, I get the following error:

Uncaught TypeError: Cannot read property 'message' of undefined
@jkrems

This comment has been minimized.

jkrems commented Jan 17, 2017

Looking at the changelog - should we be pinning to a specific version of the runtime or at least to a specific minor?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 17, 2017

Static code is still somewhat in flux due to being an entirely new feature. Hence, compatibility between minor version updates (i.e. 6.4 to 6.5) can't really be guaranteed. In such a case I'd recommend to depend on "protobuf.js": "~6.5.0" (or likewise) for now, which only accepts patch updates, or to take possibly re-generations of the code into account.

In the case of 6.4 to 6.5, converters have been upgraded to first class codegen citizens. While from works pretty much the same as before, it doesn't use a general converter (here: this.convert) anymore but uses specialized methods for each.

@jkrems

This comment has been minimized.

jkrems commented Jan 17, 2017

Cool, thanks. We'll investigate moving code generation to startup and for now we'll narrow it down to ~6.5.0.

@jkrems jkrems closed this Jan 17, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 17, 2017

Also going to think about semver-compatible versioning again with the next breaking change. If you see a protobuf.js v7 in the near future, you'll know.

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

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