8000 Fix https://github.com/dcodeIO/ProtoBuf.js/issues/19. by Dretch · Pull Request #2 · protobufjs/bytebuffer.js · GitHub
[go: up one dir, main page]
Skip to content

Fix https://github.com/dcodeIO/ProtoBuf.js/issues/19. #2

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix https://github.com/dcodeIO/ProtoBuf.js/issues/19. #2

wants to merge 1 commit into from

Conversation

Dretch
Copy link
Contributor
@Dretch Dretch commented May 11, 2013

This should fix protobufjs/protobuf.js#19.

The pre-built .js files in the root directory haven't been updated - sorry. I haven't managed to get the build working on my machine.

dcodeIO pushed a commit that referenced this pull request May 11, 2013
Fixed toBuffer, rebuild, #2 ?
10e01c1
@dcodeIO
Copy link
Member
dcodeIO commented May 11, 2013

Thank you, I have rebuilt all files and fixed a broken test case but the test you provided still fails:

AssertionError: 
'>10 02 18 00 20 80 B0 D9 B4 E8 27 28 00 00 00 00 \n 00<00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 '
==
'>10 02 18 00 20 80 B0 D9 B4 E8 27 28 93 99 8E CD \n 04<00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 '

Any ideas?

@Dretch
Copy link
Contributor Author
Dretch commented May 11, 2013

I am not sure what might be going wrong. FWIW I was running the test case by changing the line in the top of the file that reads:

var FILE = "ByteBuffer.min.js";

To read:

var FILE = "src/ByteBuffer.js";

because I had only changed the file in src.

@Dretch
Copy link
Contributor Author
Dretch commented May 12, 2013

@dcodeIO If it helps, the error you posted is exactly the error I was getting before implementing the fix.

I am hesitant to comment on this because I don't much about it, but is Travis actually using the correct file? https://travis-ci.org/dcodeIO/ByteBuffer.js/jobs/7084718 doesn't mention ByteBuffer.min.js, which makes me wonder if it is recreating it from the updated src/ByteBuffer.js?

@dcodeIO
Copy link
Member
dcodeIO commented May 12, 2013

I got the test case to work on both ByteBuffer.js and ByteBuffer.min.js. Also Travis reports a success now: https://travis-ci.org/dcodeIO/ByteBuffer.js

Can you confirm that it's working now? :)

@Dretch
Copy link
Contributor Author
Dretch commented May 12, 2013

I think the issue might still exist. I believe that the original problem was that writeVarint64 did

dst = new Uint8Array(this.array);

and next it did:

this.ensureCapacity(offset+size);

which sometimes assigns a new value to this.array, but dst still points to the old this.array... which means doing dst[...] = ... does not have the intended consequences.

As far as I can tell this issue still exists - I guess the test case in my pull request now passes because it was changed to create a ByteBuffer with an initial length of 17 (I submitted it with a length of 1), which I guess means that this.ensureCapacity(...) never needs to assign a new this.array.

Thanks for your continuing help. 👍

@dcodeIO dcodeIO closed this in 659cfd7 May 12, 2013
@dcodeIO
Copy link
Member
dcodeIO commented May 12, 2013

Thank you for pointing this out, now I understand. I've fixed it and updated the NPM packages of both ByteBuffer and ProtoBuf.js to ensure that new installations - also on node - will work correctly from now on :)

@Dretch Dretch deleted the protobuf-issue-19 branch May 28, 2013 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug resolved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Certain Long values cause "Error: Illegal field id in Message .Foo#decode: 0"
2 participants
@Dretch @dcodeIO
0