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.

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

feat!: move command line tool to a new package named protobufjs-cli #1234

Merged
merged 9 commits into from
Jan 16, 2021

Conversation

taylorcode
Copy link
Collaborator

@taylorcode taylorcode commented Jun 1, 2019

I ran into issue #716 when trying to integrate pbjs and pbts into my build system. We must mirror our dependencies for reproducible builds.

@dcodeIO recommended that we convert cli/ into a new package. I see that the work here was started, but left in an incomplete state.

This PR attempts to finish decoupling protobufjs-cli from the parent package, so that it can be installed separately and so we can remove the behind the scenes npm install.

I recommend reviewing commit by commit, I tried to keep them as single purpose as possible. Please consider this is my first time contributing to this project.

To test my changes I ran npm run test in the parent package. To test the binaries exposed by protobujs-cli package and verify that they do not depend on the parent package, I moved the /cli folder outside of the parent repo and invoked the binaries directly. I confirmed with the following commands:

pbjs --target static-module --wrap es6 protos/sample.proto

provided a proto with the content:

syntax = "proto3";

package sample_js_init_data;

message TestMessage {
    bool test_bool = 1;
    string test_string = 2;
}

It logs generated js.

When output to a file generated_js/sample.js, and running the following command:
pbts generated_js/sample.js

It logs the generated typings file content.

@taylorcode taylorcode force-pushed the cli_package branch 3 times, most recently from ad99d66 to 9883a96 Compare June 1, 2019 20:26
@taylorcode
Copy link
Collaborator Author

Hi, can I get a review of this :)? thanks

@alexander-fenster
Copy link
Contributor

This feels like a breaking change since pbjs / pbts won't be available just after installing protobufjs if I'm reading it right. I don't feel we have an authority to approve this here :) @dcodeIO can you take a look? :)

@dcodeIO
Copy link
Member

dcodeIO commented Jun 24, 2019

I'd suggest to collect all the breaking changes (iirc there are more currently blocked for similar reasons that are worth merging) and make it a new major release. A bonus might be to first collect everything not breaking into a minor, but certainly implies quite some work, so depends on how viable it is and if someone is willing to pick this up :)

@taylorcode
Copy link
Collaborator Author

@dcodeIO I'm happy to do this.

To clarify, are you suggesting:

  • make these changes in a backward compatible manner (leaving pbts and pbjs exposed on the top level package)
  • Ask you to do a minor release
  • Unexpose pbts/pbjs from top level
  • Major release

@dcodeIO
Copy link
Member

dcodeIO commented Jun 25, 2019

Suggestion was to do backward compatible fixes first, leading to a new minor release (6.8.9), and afterwards do anything breaking leading to a new major release (7.0.0, switch to semver going forward). I'm not sure how important doing a minor update first is, though. Might be that it's not worth it.

@taylorcode taylorcode force-pushed the cli_package branch 3 times, most recently from 1edda53 to ff89508 Compare August 7, 2019 00:32
@taylorcode
Copy link
Collaborator Author

Hey @dcodeIO I made one small improvement to my changes to report an error if protobufjs can't be resolved from the protobufjs-cli package.

I looked over the code again and I don't think that it makes sense to do a minor release because all of the changes are related to separating out the protobufjs-cli package, and the non-breaking changes do not add any value by themselves.

The non-breaking changes include:

  • resolving protobufjs from the parent directory or from the node_modules folder
  • adding the package.json file to /cli and removing package-standalone.json

If this sounds reasonable to you, how can we proceed? Should I increment the version number to 7.0.0 for both protobufjs and protobufjs-cli?

@dcodeIO
Copy link
Member

dcodeIO commented Aug 7, 2019

If this sounds reasonable to you, how can we proceed? Should I increment the version number to 7.0.0 for both protobufjs and protobufjs-cli?

A few questions:

  • Is cli/resolve-protobufjs.js the recommended way of resolving peer deps or did you come up with it? In the latter case, is there maybe a recommended pattern?
  • Some changes to package-lock's might be unnecessary, for example lib/eventemitter doesn't really need one. Should be maybe limit package-lock's in source control to those packages with external non-dev dependencies?

Overall this PR looks good to me, but I didn't test it locally. Would be great if multiple people could verify that it works without issues. Incrementing the version here makes sense to me as well, even if we might collect additional PRs before doing an actual release.

@taylorcode
Copy link
Collaborator Author

Is cli/resolve-protobufjs.js the recommended way of resolving peer deps or did you come up with it? In the latter case, is there maybe a recommended pattern?

In my opinion the best way to solve this problem is to use Lerna or Yarn workspaces for developing multiple packages in the same repo. If we used Lerna we would be able to reference protobufjs by the module name "protobufjs" for both modes (local development and installed as an npm package). I've used Lerna for projects in the past and it works well.

I'm happy to follow up after this PR and see what it would take to move this repo to Lerna.

Some changes to package-lock's might be unnecessary, for example lib/eventemitter doesn't really need one.

yep, nice catch. The package-lock.json for eventemitter was added by accident, I must have run npm i inside of that folder.

Why do you feel that the package-lock.json is unnecessary? If we decided to use Lerna, we would have a package-lock for each subpackage, and lerna would manage updating them.

Would be great if multiple people could verify that it works without issues.

I will post my manual test plan here. Any automated tests you recommend I am happy to add as well.

Incrementing the version here makes sense to me as well, even if we might collect additional PRs before doing an actual release.

I updated it to 7.0.0

@taylorcode taylorcode force-pushed the cli_package branch 2 times, most recently from f8ac088 to 6a8dc73 Compare August 7, 2019 21:40
@taylorcode
Copy link
Collaborator Author

taylorcode commented Aug 7, 2019

To test my changes:

I published protobufjs-taylorm and protobufjs-cli-taylorm as npm packages, created a new folder with the following package.json:

{
  "name": "protobufjs_fork_tests",
  "version": "1.0.0",,
  "scripts": {
    "pbjs_test": "pbjs --target static-module --wrap es6 sample.proto > sample.js",
    "pbts_test": "pbts sample.js"
  },
  "dependencies": {
    "protobufjs-cli-taylorm": "^7.0.4",
    "protobufjs-taylorm": "^7.0.4"
  }
}

(note that 7.0.4 was necessary because I already published prior versions to integrate it into my project.)

And sample.proto:

syntax = "proto3";

package sample_js_init_data;

message TestMessage {
    bool test_bool = 1;
    string test_string = 2;
}

I then ran npm run pbjs_test and npm run pbts_test. The first generated sample.js, the second output the .d.ts file content to my console.

To test inside of the repo, I added sample.proto inside of cli/, and invoked the binaries directly with the same arguments.

@taylorcode
Copy link
Collaborator Author

Hi @alexander-fenster, @dcodeIO, can I have your review? thanks

cli/package.json Outdated
"name": "protobufjs-cli",
"description": "Translates between file formats and generates static code as well as TypeScript definitions.",
"version": "7.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to confirm that we want to start this separate package from v7.0.0 - it's up to @dcodeIO to decide.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @dcodeIO can you advise here so that we can get this merged, if all else looks good. Thanks

cli/package.json Outdated Show resolved Hide resolved
@taylorcode
Copy link
Collaborator Author

Hey @dcodeIO and @alexander-fenster, my team has another improvement to protobuf.js that is based on this fork.

Should we wait to open a new PR until this is merged in? Thanks

@alexander-fenster
Copy link
Contributor

@taylorcode I cannot create new release (I work for a Google team that works on gRPC, client libraries, and protobuf, so we asked for write access here to help with stuff, but I cannot and don't want to make releases :) ) - it's all up to Daniel @dcodeIO as for when to merge and release.

Having that said - as a user of pbjs/pbts, let me try playing with this locally to verify all this stuff works as expected. I'll write back when I check it.

@taylorcode
Copy link
Collaborator Author

taylorcode commented Aug 22, 2019

sounds good thanks @alexander-fenster. Thanks for helping to verify that it works. I'll wait for @dcodeIO, and just have my teammate open a new PR after this one lands.

@taylorcode
Copy link
Collaborator Author

taylorcode commented Aug 27, 2019

hey @dcodeIO can you take another look?
also @alexander-fenster were you able to do some local testing? Thank you both

@taylorcode
Copy link
Collaborator Author

hey folks... can I have someone review this?, it's been more than 3 months since I opened this PR.

@taylorcode
Copy link
Collaborator Author

Hi @dcodeIO reminder on this PR. I’m happy to make any changes.

My team has a few other fixes that we are working on and would like to merge in instead of maintaining our own fork.

@alejandroclaro
Copy link

Any news about the release date?

@Justme0
Copy link

Justme0 commented Oct 21, 2021

After install protobufjs-cli, run cmd failed. It's same as #877 (comment) How to solve it?

install ok

$ sudo npm i   -g protobufjs
/usr/local/bin/pbjs -> /usr/local/lib/node_modules/protobufjs/bin/pbjs
/usr/local/bin/pbts -> /usr/local/lib/node_modules/protobufjs/bin/pbts

> protobufjs@6.11.2 postinstall /usr/local/lib/node_modules/protobufjs
> node scripts/postinstall

/usr/local/lib
└── protobufjs@6.11.2
$ pbjs -t static_module  -o a.js a.proto
installing chalk@^4.0.0
installing jsdoc@^3.6.3
installing minimist@^1.2.0
installing uglify-js@^3.7.7
child_process.js:660
    throw err;
    ^

Error: Command failed: npm --silent install chalk@^4.0.0 jsdoc@^3.6.3 minimist@^1.2.0 uglify-js@^3.7.7
    at checkExecSyncError (child_process.js:621:11)
    at Object.execSync (child_process.js:657:15)
    at modInstall (/usr/local/lib/node_modules/protobufjs/cli/util.js:129:19)
    at Object.exports.setup (/usr/local/lib/node_modules/protobufjs/cli/util.js:156:5)
    at Object.<anonymous> (/usr/local/lib/node_modules/protobufjs/cli/pbjs.js:7:6)
    at Module._compile (internal/modules/cjs/loader.js:936:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:947:10)
    at Module.load (internal/modules/cjs/loader.js:790:32)
    at Function.Module._load (internal/modules/cjs/loader.js:703:12)
    at Module.require (internal/modules/cjs/loader.js:830:19) {
  status: 243,
  signal: null,
  output: [ null, null, null ],
  pid: 6323,
  stdout: null,
  stderr: null
}

@seansfkelley
Copy link

What's the status of this change's release? It's been 18 months since this PR was closed and the bot keeps opening and closing release PRs that include this feature -- at the time of writing, there are two concurrent pending release tasks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.