search

marook / osm-read

an openstreetmap XML and PBF data parser for node.js and the browser
GNU Lesser General Public License v3.0
107 stars 25 forks source link

load generated js proto files #7

Closed nrenner closed 10 years ago

nrenner commented 10 years ago

part of #4

Generate commonjs modules from proto files with proto2js and put all in a proto subdirectory. The js files are easier to package/load for the browser, and also don't need the proto parsing step at runtime.

Suggesting to also use these modules for Node.js.

marook commented 10 years ago

Do I get it right that the .proto files have been transformed (by using npm run buildProto) into some JSON object? And now instead of parsing the .proto files the library uses the generated JSON to build the protobuf definitions?

I guess you might be right that the parsing of the JSON representation should be slightly faster than parsing the .proto files.

nrenner commented 10 years ago

Do I get it right that the .proto files have been transformed (by using npm run buildProto) into some JSON object?

Yes, the .proto files have been parsed to their JSON representation using the proto2js commandline tool. The -commonjs option generates a JavaScript wrapper around the JSON that calls the builder and exports it as a CommonJS module. This can be required in Node.js and also bundled for the browser with browserify.

And now instead of parsing the .proto files the library uses the generated JSON to build the protobuf definitions?

Right, this way the parsing step is done only once at build time instead of everytime at runtime.

I guess you might be right that the parsing of the JSON representation should be slightly faster than parsing the .proto files.

It does not seem to be a lot faster, so I think it doesn't make much of a difference. But the PBF definition doesn't really change a lot, so there is no need for runtime parsing.

The main benefit is with the browser, because the modules can be bundled with browserify instead of doing some Ajax loading. Also, the smaller ProtoBuf.noparse.js package could be used, see Builder docs.

I just thought it would be nice to also use this for the Node.js part because it reduces the .proto loading code. But if you prefer the previous .proto file loading variant, I'm fine with splitting this into two separate implementations.

marook commented 10 years ago

I just thought it would be nice to also use this for the Node.js part because it reduces the .proto loading code. But if you prefer the previous .proto file loading variant, I'm fine with splitting this into two separate implementations.

I think we should also use the JSON representation for node.js. Maintaining two different implementations doesen't make not sense in my opinion.

Thank you very much for all the very cool contributions!

nrenner commented 10 years ago

Thanks!