-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Use protobuf/minimal when pbjs target is static-module #813
Use protobuf/minimal when pbjs target is static-module #813
Conversation
@@ -99,6 +99,8 @@ exports.main = function main(args, callback) { | |||
" es6 ES6 wrapper (implies --es6)", | |||
" closure Just a closure adding to protobuf.roots (see -r)", | |||
"", | |||
" --dependency Specifies which version of protobuf to require. Accepts any valid module id", | |||
"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, $DEPENDENCY
is defined by the respective target and if I am not mistaken it is not (yet) a used argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood your concern correct, it is actually used in util.wrap
called by static-module
and json-module
targets. This option gets correctly passed down to (json|static)-module
target.
I only see one concern at the moment: static-module
target sepcifies default value for dependency
, but json-module
- doesn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be more specific. util.wrap uses protobufjs
as default value when dependency option is not provided.
static-module
target will use protobufjs/minimal
as default if not overwritten by options, provided in arguments, but json-module
is not doing this.
To make things consistent we can either remove default value from static-module
code and specify default in pbjs.js
or update json-module
target
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, the argument should always be honored in case the user has a legitimate reason to override it, but the targets should have their defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it will. dependency
will be passed inside of options object to target. Target will set dependency
to protobufjs/minimal
if not provided externally, and will pass options to util.wrap
.
c32c5f9
to
4f74067
Compare
4f74067
to
4eac28c
Compare
Rebased onto master |
"minimal" doesn't include `Root`, as mentioned in protobufjs#828 and protobufjs#856. Probably caused by protobufjs#813.
"minimal" doesn't include `Root`, as mentioned in protobufjs#828 and protobufjs#856. Probably caused by protobufjs#813.
This should fix #798
Change affects AMD wrapper too, to be consistent.
Some modifications to AMD loader configs may be required, i.e.
protobufjs/minimal
should be mapped to correct file. Other option is to passdependency
argument to pbts and override default module name.