brianchirls / proto-loader

Protocol Buffer loader module for webpack
27 stars 18 forks source link

Cut the require protobufjs step #4

Open samuelmesq opened 9 years ago

samuelmesq commented 9 years ago

It would be better if you could skip all this part of require('protobufjs'), in my tests i could not, some problems with the protobufjs. basically what I tried to do was change the file ending to use the protobufjs:

return 'var protobuf = require("protobufjs"); module.exports = protobuf.loadJson(...'

In theory this works, right? But the protobufjs was lost in their own dependencies :+1:

Someone see better ways of doing this?

brianchirls commented 9 years ago

Yeah this is totally possible.

I don't remember exactly why I did it the way I did. I suppose in theory, there might be something else you'd want to do with the definition object or with the builder. Like this maybe?

@sindrenm, @watsoncj what do you guys think?

watsoncj commented 9 years ago

Webpack won't build protobufjs 4.0.1+ due to the way they pass the require function around. There is also a reference to Long that doesn't properly map to the long package. I suspect browserify will not build it either.

I got around this by using the script-loader to include protobufjs, bytebuffer and Long. The externals declaration then allows one to require('protobufjs').

// webpack.config.js
module.exports = {
  entry: [
    'script!./node_modules/protobufjs/node_modules/bytebuffer/node_modules/long/dist/Long.min.js',
    'script!./node_modules/protobufjs/node_modules/bytebuffer/dist/ByteBufferAB.min.js',
    'script!./node_modules/protobufjs/dist/ProtoBuf-light.min.js',
    './app/scripts/app.js'
  ],
  ...
  externals: {
    dcodeIO: 'dcodeIO',
    protobufjs: 'dcodeIO.ProtoBuf',
    bytebuffer: 'dcodeIO.ByteBuffer'
  },
  ...
};
brianchirls commented 9 years ago

@watsoncj: Are you sure? Can you take a look at the example on the skip-require-protobufjs branch? It seems to build just fine for me, now that I've upgraded protobufjs.

watsoncj commented 9 years ago

Thanks for checking, @brianchirls!

Webpack of protobufjs was just fixed in 4.1.2.

Sorry for the tangent, I misunderstood the original intent of this issue. The simplified usage pattern looks great!

brianchirls commented 8 years ago

I'm gonna punt on this one a little longer, and possibly forever, because protobufjs provides a "light" build that doesn't include code for parsing .proto files. You require it like this:

var ProtoBuf = require('protobufjs/dist/protobuf-light')

I assume most people using this loader would want to use the light script, but I don't want to force it on anyone.

watsoncj commented 8 years ago

Sounds like a wise decision to keep things flexible and agnostic.

The boilerplate require can always be wrapped up in an application-side utility function.

brianchirls commented 8 years ago

Yeah, you could do it that way, but be careful because if you have the require call with a variable, webpack might try to include a whole directory in your build...I think.

juliostanley commented 8 years ago

I'd suggest that much like you import less or sass you import the proto.

So another module named protobuf-loader, would enter the Jain as:

// without a loader config var builder = require('protobuf!proto!message.proto');

One of us can create a repo for protobuf-loader.

Hope i didnt miss the point of this issue, Thoughts?