Use protobuf/minimal when pbjs target is static-module#813
Conversation
| " 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.
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.
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.
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.
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.
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.
betalb
force-pushed
the
use-minimal-version-when-bunling-modules
branch
2 times, most recently
from
c32c5f9 to
4f74067
Compare
betalb
force-pushed
the
use-minimal-version-when-bunling-modules
branch
from
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.
3 participants
This should fix #798
Change affects AMD wrapper too, to be consistent.
Some modifications to AMD loader configs may be required, i.e.
protobufjs/minimalshould be mapped to correct file. Other option is to passdependencyargument to pbts and override default module name.