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.

Sign up for GitHub

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

Jump to bottom

Use protobuf/minimal when pbjs target is static-module #813

Merged
merged 1 commit into from
May 30, 2017
Merged

Use protobuf/minimal when pbjs target is static-module #813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cli/pbjs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ exports.main = function main(args, callback) {
"force-long": "strict-long",
"force-message": "strict-message"
},
string: [ "target", "out", "path", "wrap", "root", "lint" ],
string: [ "target", "out", "path", "wrap", "dependency", "root", "lint" ],
boolean: [ "create", "encode", "decode", "verify", "convert", "delimited", "beautify", "comments", "es6", "sparse", "keep-case", "force-long", "force-number", "force-enum-string", "force-message" ],
default: {
target: "json",
Expand Down Expand Up @@ -101,6 +101,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",
"",
Copy link
Member

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.

Copy link
Author

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

Copy link
Author

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

Copy link
Member

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.

Copy link
Author

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.

" -r, --root Specifies an alternative protobuf.roots name.",
"",
" -l, --lint Linter configuration. Defaults to protobuf.js-compatible rules:",
Expand Down
4 changes: 3 additions & 1 deletion cli/targets/json-module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ module.exports = json_module;

var util = require("../util");

var protobuf = require("../..");

json_module.description = "JSON representation as a module";

function json_module(root, options, callback) {
Expand All @@ -17,7 +19,7 @@ function json_module(root, options, callback) {
}
var json = util.jsonSafeProp(JSON.stringify(root.nested, null, 2).trim());
output.push(".addJSON(" + json + ");");
output = util.wrap(output.join(""), options);
output = util.wrap(output.join(""), protobuf.util.merge({ dependency: "protobufjs/minimal" }, options));
process.nextTick(function() {
callback(null, output);
});
Expand Down
2 changes: 1 addition & 1 deletion cli/wrappers/amd.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
define(["protobuf"], function($protobuf) {
define([$DEPENDENCY], function($protobuf) {
"use strict";

$OUTPUT;
Expand Down
2 changes: 1 addition & 1 deletion cli/wrappers/default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
(function(global, factory) { /* global define, require, module */

/* AMD */ if (typeof define === 'function' && define.amd)
define(["protobuf"], factory);
define([$DEPENDENCY], factory);

/* CommonJS */ else if (typeof require === 'function' && typeof module === 'object' && module && module.exports)
module.exports = factory(require($DEPENDENCY));
Expand Down
2 changes: 1 addition & 1 deletion cli/wrappers/es6.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import * as $protobuf from "protobufjs";
import * as $protobuf from $DEPENDENCY;

$OUTPUT;

Expand Down