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

BSON Decoding of Binary Objects #185

Merged
merged 4 commits into from
Jul 16, 2015
Merged

BSON Decoding of Binary Objects #185

merged 4 commits into from
Jul 16, 2015

Conversation

DLu
Copy link
Contributor

@DLu DLu commented Jul 10, 2015

roslibjs component of RobotWebTools/rosbridge_suite#169

Pairs with RobotWebTools/rosbridge_suite#190

Current problem: BSON is a huge dependency.

Original file sizes

-rw-rw-r-- 1 dlu dlu  65K Jul 10 17:08 roslib.js
-rw-rw-r-- 1 dlu dlu  27K Jul 10 17:09 roslib.min.js

New File Sizes with this PR

-rw-rw-r-- 1 dlu dlu 275K Jul 10 17:09 roslib.js
-rw-rw-r-- 1 dlu dlu 104K Jul 10 17:09 roslib.min.js

Is that a problem? Is there a better way?

@DLu
Copy link
Contributor Author

DLu commented Jul 10, 2015

Also, do you need to check in the updated built files? I didn't and I'm trying to tell if that's why the tests are failing.

@rctoris
Copy link
Contributor

rctoris commented Jul 13, 2015

You don't need to rebuild in on a PR. The tests seem to be failing because it can't load Blob. You should do a check to see if Blob is actually available https://github.com/RobotWebTools/roslibjs/pull/185/files#diff-49dcdae7a46fbde852bf41066508c9f7R111

@DLu
Copy link
Contributor Author

DLu commented Jul 15, 2015

That was easier than I originally thought. I was trying to find the correct dependency to ensure Blob was defined. I think I did it correctly this time. Javascript is a weird language.

The original question about the file size remains.

@rctoris
Copy link
Contributor

rctoris commented Jul 15, 2015

Is this pulling in all of bson into the build file? Can we make it a shim like EventEmitter2 and such? I'm less familiar with the idea but I think this is the way to go.

https://github.com/RobotWebTools/roslibjs/tree/develop/src/util/shim
https://github.com/RobotWebTools/roslibjs/blob/develop/package.json#L38

@DLu
Copy link
Contributor Author

DLu commented Jul 15, 2015

Ok, I've gotten it to the point where it works and builds. It will work correctly if you include the [https://raw.githubusercontent.com/mongodb/js-bson/master/browser_build/bson.js](bson header) and print an error message if you try to decode a bson message without the header.

However, I don't think this is the right way to do it. I tried to make a shim, but had problems getting it to build with grunt AND work in the browser.

@rctoris
Copy link
Contributor

rctoris commented Jul 15, 2015

Since it is an optional feature we can make it optional like ColladaLoader is in ros3djs

rctoris added a commit that referenced this pull request Jul 16, 2015
BSON Decoding of Binary Objects
@rctoris rctoris merged commit 3e25e4e into RobotWebTools:develop Jul 16, 2015
@megawac
Copy link
Contributor

megawac commented Jul 16, 2015

Are we not going to move this to an extension adapter, so the user could include if desired, to avoid the file size increase

require('roslib/adapters/bson')

@DLu DLu deleted the bson branch July 20, 2015 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants