dgp1130 / bxpb

Browser Extension Protocol Buffer Client/Service Implementation
MIT License
0 stars 0 forks source link

MVP #1

Closed dgp1130 closed 4 years ago

dgp1130 commented 4 years ago

This issue tracks the mainline work to be done to launch minimum viable product (MVP). It gives me somewhere to write down general thoughts and notes that don't necessarily relate to commit messages or markdown docs.

In my view, MVP requires:

Edit: All the major requirements are currently complete, but there are a few cleanup tasks still to do before the 1.0.0 release.

Requirements explicitly not covered by MVP:

dgp1130 commented 4 years ago

Quick rant about Lerna, I've been trying really hard to use existing Node tooling for this project, but I'm still not satisfied. I was hoping I could use Lerna to link local dependencies together and then use lerna run build to build them in the correct order. This seems to work ok, however with testing it kind of falls apart.

The possible ways I can test a package include:

Debugging is weird too, because I need to set a node port on a deeply nested command. Add to this that npm test is a combination of tsc (to build the test code) and jasmine (to execute it). This means that to debug I would need something like:

npm run lerna run build --scope packages/foo/ --include-dependencies && npm run lerna run test:build --scope packages/foo/ && npm run lerna exec node --scope packages/foo/ -- node --inspect=${DEBUG_PORT} node_modules/.bin/jasmine

Then the package.json would need to be:

{
  "scripts": {
    "test": "npm run -s test:build && jasmine",
    "test:build": "tsc -p tsconfig.spec.json"
  }
}

The final exec command is needed to put the Node port exactly where it's needed and breaks abstraction in a really ugly way. I also need to separate the test build step just so it can be manually run for the package before executing the test. I'd also need to set this up separately for VSCode using multiple tasks, and preLaunchTasks which also break abstractions and repeat the build/test infrastructure.

TL;DR: I don't see an elegant way of managing/running tests with Lerna which I'm happy with. Maybe there's something I'm missing. I suspect Karma would solve a few of these problems, but mainly just by sidestepping them rather than Lerna really doing what I want? I'll see if I can get a decent debug flow with Karma, and if not, I might swap out Lerna for Bazel instead.

dgp1130 commented 4 years ago

I just spent a couple hours trying to compile a .proto file only to eventually determine this to be infeasible. AFAICT, Google has no supported mechanism of generating TypeScript-compatible code with protobufs. There are a number of community-built alternatives, but I wanted to stick with Google-supported tools as much as possible.

I tried to generate JavaScript protos instead, and then run them through Clutz to generate TypeScript definitions. I believe this is how it works in google3, so I figured that was closest I could get to a directly supported option. Unfortunately, this doesn't really seem to work either. Closest I got was this command:

grpc_tools_node_protoc --js_out=import_style=commonjs,binary:test-out/test_data/ --proto_path src/test_data/ src/test_data/*.proto
clutz test-out/test_data/*.js --closure_env BROWSER --externs ../../closure/contrib/nodejs/globals.js
test-out/test_data/greeter_pb.js:27: ERROR - [JSC_UNDEFINED_VARIABLE] variable proto is undeclared
  25|  * @constructor
  26|  */
  27| proto.foo.bar.GreetRequest = function(opt_data) {
  28|   jspb.Message.initialize(this, opt_data, 0, -1, null, null);
  29| };

test-out/test_data/greeter_pb.js:31: ERROR - [JSC_UNDEFINED_VARIABLE] variable COMPILED is undeclared
  29| };
  30| goog.inherits(proto.foo.bar.GreetRequest, jspb.Message);
  31| if (goog.DEBUG && !COMPILED) {
  32|   proto.foo.bar.GreetRequest.displayName = 'proto.foo.bar.GreetRequest';
  33| }

test-out/test_data/greeter_pb.js:300: ERROR - [JSC_UNDEFINED_VARIABLE] variable exports is undeclared
  298| 
  299| 
  300| goog.object.extend(exports, proto.foo.bar);

3 error(s), 0 warning(s)

The first conflict is about the proto import. The generated proto JavaScript has a require('google-protobuf') which should define that symbol. The proto namespace is generated by individual proto files, so there's no library/runtime to link to here. The fact that jspb.Message resolves, makes me think the require('google-protobuf') worked. I'm not sure why proto. doesn't. This might be that proto files generate with goog.exportSymbol() rather than goog.provide() or goog.module(). The latter two actually create the object at that namespace, while goog.exportSymbol() does not (per docs).

The second conflict is COMPILED. This comes from base.js, but trying to add it causes a name conflict for goog because that is also defined in the protobuf JavaScript for some reason. This is just a boolean, so I could manually declare it if absolutely necessary.

The third conflict seems to be exporting the proto. I added Node externs to handle CommonJS symbols (ie. require()), but this didn't work for export. I'm not entirely sure why, but it might be necessary to set --process_common_js_modules per this doc. Unfortunately, Clutz wraps Closure and does not appear to expose a hook to that flag.

See https://github.com/angular/clutz/issues/334 for more info.

I could keep debugging and just try to get Closure itself to compile the generated protobuf, then worry about Clutz afterwards. Maybe I'll take another crack at this later, but it seems like Clutz and Closure are not effectively working together here. If I can't get anything usable from this, I might just have to fall back to the community alternatives.

dgp1130 commented 4 years ago

I took another quick pass at Clutz, but didn't have much luck. I can get Closure to compile if I use compilation_level=SIMPLE, but I think that just turns off a lot of the type checking, which is necessary for Clutz.

(cd packages/bxpb-runtime/ && npx google-closure-compiler --js test-out/test_data/greeter_pb.js --compilation_level SIMPLE)

I tried the --process_common_js_modules flag on Closure, but that isn't able to resolve the require() call either.

$ (cd packages/bxpb-runtime/ && npx google-closure-compiler --js test-out/test_data/greeter_pb.js --js node_modules/google-protobuf/**/*.js --js node_modules/google-protobuf/package.json --compilation_level ADVANCED --process_common_js_modules --module_resolution NODE)

test-out/test_data/greeter_pb.js:10: ERROR - [JSC_UNDEFINED_VARIABLE] variable module$node_modules$google_protobuf$google_protobuf is undeclared
var jspb = require('google-protobuf');
                   ^^^^^^^^^^^^^^^^^

test-out/test_data/greeter_pb.js:27: ERROR - [JSC_UNDEFINED_VARIABLE] variable proto is undeclared
proto.foo.bar.GreetRequest = function(opt_data) {
^^^^^

There are also unrelated errors directly in google-protobuf code.

I also tried changing the output format of protoc to --js_out=import_style=closure,binary:out/. This generates Closure-compatible code as expected, but fails because jspb.* is not provided. google-protobuf doesn't export this to node_modules/, so it looks like I'd need to link directly into the source code, via a git submodule or some other trick.

This is just becoming a big waste of time. Clutz, Closure, and protobufs are just too unergonomic as to be effectively unusable in the OSS ecosystem. For now, I think I'll use some community resources to generate TypeScript code and worry about potential google3 compatibility if/when that becomes relevant.

dgp1130 commented 4 years ago

I have the runtime mostly working and was able to set up an example app which used it to implement and call a protobuf service in a Chrome extension using chrome.runtime.sendMessage/chrome.runtime.onMessage. I had to hand-write the client and service stubs as the protoc plugin doesn't exist yet, but this is good progress and validates the approach so far. That actually satisfies three of the four initial requirements already!

So far I've mostly learned that protocol buffers are incredibly unwieldy in web dev projects simple due to the build pipeline. Web projects already have a lot of complexity between compilers (tsc), packages (NPM/Pika/etc.), bundlers (Rollup/Webpack/etc.), formats (CommonJS/ESM/etc.) orchestrators (Webpack, NPM scripts), and more. Protobuf tooling wasn't originally designed for the web ecosystem and it definitely shows. It certainly can be made to work, but it is difficult and frustrating.

Protobufs don't support ESM yet, so we're forced into CommonJS which also means we're forced into a bundler. I was hoping to have an example which was simple enough to not need any of those, but I don't think I can quite get away with that.

Developers seem to be expected to manually install protoc and put it on the path. I did find grpc-tools which seems to install and export a protoc binary, but this isn't well documented and doesn't appear to be the intended usage of protoc for web projects. It's also inherently coupled to gRPC, which this project is totally unrelated to and should not have a dependency on, but we're kind of forced into it anyways.

Even once MVP is ready, there is only so easy I can make the set up process and only so well I can document it. Any user who tries to leverage this project is immediately going to run into the "How do I compile protobufs" problem, which is quite difficult and hard to get buy-in for. Unless they have used protobufs before, or are using something like Bazel (also not well-loved in the web space), most devs simply would not go through the pain and suffering to make it happen.

Ultimately there's not a whole lot I can do about this besides documenting it as well as possible and try to ease the onboarding experience as best I can. I could try contributing to protobufs to better support this, but it doesn't have much open-source support by Google, particularly for web. I think that the ideal developer experience includes one or more of the following tools depending on the project using it:

Some of these exist in community packages, but using those pulls you further away from the core protobuf system. Community tools shouldn't be needed in the first place as most of these are table stakes for modern web dev. Anyways, the takeaway here is that the biggest negative I see for a potential client adopting this package is because the existing developer experience for protos in web is so crappy. Post-MVP, I may need to spend some time contributing to the protobuf team as it may be the most impactful way of improving this project.

The next steps mostly come down to building out the protoc plugin. I also want to change the service API to just export a function from the generated code which hard-codes the service descriptor in it. That way, users only need to import the generated code and don't need to have the mental overhead of understanding WTF a "service descriptor" is. This could be as simple as:

import { serveGreeter } from './protos/my_service_bxpb';
import { GreetRequest, GreetResponse } from './protos/_my_service_pb';

serveGreeter(chrome.runtime.onMessage, {
  async Greet(req: GreetRequest): Promise<GreetResponse> {
    return new GreetResponse();
  },
});

If I have time and it would be easy, I would love to get some real tests on the example app. I would need to set up Puppeteer or Playwright to install the example Chrome extension, focus on the popup, send a request, and then assert the response. That sounds like a pain in the ass to set up, so it might not be worth the effort just yet. I think I'll take a look at that, but if it turns out to be difficult, I'll probably punt until after MVP.

The major next step is the protoc plugin. That should allow us the generate the client/service stubs and remove the hand-written ones from the example. After that, it mostly comes down to documentation and maybe a bit of marketing.

dgp1130 commented 4 years ago

At this point all the major requirements are supported. The library is technically in a releaseable state.

Before I actually release version 1.0.0, there are a few remaining tasks I want to take care of. This is mostly to make sure I don't immediately follow up with a 2.0.0 release in order to fix something I should have considered before the initial release.

Edit: Crossed out items have been ignore/skipped due to https://github.com/dgp1130/bxpb/issues/1#issuecomment-639994214.

dgp1130 commented 4 years ago

I looked into making an @bxpb/ NPM org, and apparently it is free as long as the packages are public, which works for me! I created the organization and renamed the packages to use the new NPM scope.

https://www.npmjs.com/org/bxpb

dgp1130 commented 4 years ago

I was looking into refactoring @bxpb/runtime to include a "main" file that re-exports all public symbols. While doing that I encountered some strange type errors and spent a long time debugging them before discovering that generated types for service implementation had a couple mistakes which caused serveGreeter() to accept any. I was able to fix them in https://github.com/dgp1130/bxpb/commit/7ee936c6092d09f049cdf6316a186dbf4e4fe686 and have full details in that commit message. I did want to add one other takeaway upon reflection which I think is the most important aspect but did not hit me until after I pushed the commit.

The biggest lesson from the relevant TypeScript issue (https://github.com/microsoft/TypeScript/issues/36971) is that you should not hand-write .d.ts files. When I was first designing BXPB, I considered generating .ts files rather than .d.ts and .js files. I decided against this because I wanted the library to be usable for JavaScript projects as well without requiring them to go through a TypeScript compile step. Even though we are generating such .d.ts files, they are still effectively "hand-written" (read: not emitted by tsc). Manually writing .d.ts files has many unintuitive side-effects not immediately obvious as described in https://github.com/dgp1130/bxpb/commit/7ee936c6092d09f049cdf6316a186dbf4e4fe686. I think the biggest lesson is that humans should not author .d.ts files (which is weird cause DefinitelyTyped does that pretty deliberately, but that's a bit of a special case). Instead, it is tsc's responsibility to emit .d.ts files from .ts source files.

Thinking back, if we had emitted real .ts files, they would have thrown relevant errors for all the mistakes made in them and would have generated the correct output .d.ts. I think the real fix for this kind of error is to generate actual .ts files and compile them, instead of generating .d.ts files.

In our compiler, we could generate .ts files and then run tsc ourselves on them to generate .js and .d.ts files. However, this is trickier than it sounds as the BXPB compiler is simply a plugin to protoc, and it is not responsible for actually writing files. It is only responsible for returning a map of file path -> file content which protoc is then responsible for actually generating. The level of abstraction doesn't lend itself for a plugin to call an external tool which interacts with the real file system.

I think the proper solution here is to use typescript as a dependency in the compiler to emit an actual AST of source code and generate it. I'm not sure how well the APIs support that particular kind of use case of a compilation which is stored entirely in-memory. If not, we could alternatively generate .ts source files in a temp directory, and then invoke tsc on it, then read back the output files and return them to protoc. That's a bit hacky and roundabout (it also might cause problems for integration with Bazel, as it typically doesn't like files writing to undeclared outputs).

I filed #4 to make this infrastructure change. I'm not convinced that this is necessary for MVP, but I would really like to avoid such complicated and in-depth debugging like this again.

dgp1130 commented 4 years ago

I've been thinking a lot about this project over the past couple days and have slowly convinced myself that it cannot be successful in the current ecosystem. I'm going to try to put my thoughts down coherently and explain my change of heart:

The problem

I was always a bit frustrated and disappointed with the end developer experience I am able to provide. I've talked before about how protocol buffers are a pain to use with current web tooling, but something more has been bothering me that I was never quite able to identify until now. The core problem with protocol buffers in the context of BXPB is that they require the buy-in of the entire data model system of a given application.

Conveniently calling a function with BXPB

What I mean by this is that in order to conveniently call a given function using BXPB, an application needs to already be able to express its core data model in protocol buffers. If I have a method like driveCar(), I first need to define Car via protocol buffers. That requires me to define a driver as a Person, a Company that owns the car, and any other data needed for my application. This will quickly grow to encompass the entire data set of the application.

In this scenario my logic comes roughly follows:

  1. BXPB is about making it trivial to "call a function" in another context of a browser extension.
  2. "Trivially calling a function" means that it must be trivial to provide arguments to the function.
  3. "Trivially providing arguments to a function" means that any object must be trivially serialized/deserialized.
    • "Any object" here is limited to fundamentally serializable objects (there's the "law of physics" limitation about what data is and is not serializable in the first place).
  4. "Trivially serializing/deserializing an object" means that all its components must also be trivially serializable/deserializable.
  5. Any sufficiently complex application will compose all its data models (objects) together in a hierarchical fashion.
    • i.e. A Car object is composed of a Person driver, a Company owner, a Manufacturer, etc. This eventually grows to encompass all objects in the system because: why would you use them together if they didn't relate to each other?
  6. Per the last two points, any sufficiently complex application must trivially serialize/deserialize all its data models.
  7. To be "trivially serializable/deserializable", an object must be expressed and converted to/from protocol buffers.
  8. Per the last two points, any sufficiently complex application must make all its data models expressible and convertible to/from protocol buffers.

QED

If there is a flaw in this proof, I think it is that using BXPB does not inherently require an application to define its entire set of data models in protocol buffers. Whatever operation is being performed via RPC may be simple and limited enough that a relatively small subset of the data model needs to be expressed. Such an application could still benefit from BXPB, but would not be likely to accept the onboarding cost of setting up protocol buffers, learning and using them correctly, and getting the required organization buy-in.

How users should use BXPB

The ideal BXPB user already uses protocol buffers for their entire existing data model and wants to leverage them to easily call functions across multiple contexts in a browser extension.

How users will actually use a BXPB

Practically speaking, few if any browser extensions actually have protocol buffer-definitions for their entire data set and uses them uniformly throughout the application.

The different between the starting point of "no protobufs" to the end state of "all protobufs" force an application to choose among some very undesirable options:

  1. Define your data models entirely in protocol buffers and only use those representations everywhere in your app.
  2. Duplicate data models in the protocol buffer IDL, but keep existing JS models around, and include serialize()/deserialize() functions for converting between them.
  3. Use protobufs only to wrap other, more convenient representations (i.e. JSON).

The first option requires total buy-in of the application into protocol buffers. Unless you are already using protobufs heavily in your application and call to gRPC backends, this is almost certainly not worth the benefit. If you already have non-protobuf data models, you'd need to migrate them to this point and would be a significant amount of effort.

Don't forget that the primary selling point of protocol buffers if that they provide strong forwards and backwards compatibility guarantees between client and service. However a browser extension is typically released monolithically, so the entire codebase is built at a single point in time, with no version skew between components. As a result, protobuf compatibility is effectively worthless to browser extensions. The only real use case I see are for calling gRPC backends, where version skew does become a meaningful problem. Hypothetically, an extension could become large and complex enough that a team might choose to more strongly version different components owned by different teams, but at that point the system is so large and complex that the team would likely develop and maintain their own RPC system rather than rely on one supported by only a single open-source developer.

The second option requires duplicating a significant amount of code with manual gluing between the two. It's a hack at best and unmaintainable at worst.

The third option would effectively define all your methods as:

message GreetRequest {
    string json = 1;
}

message GreetResponse {
    string json = 1;
}

service Greeter {
    rpc Greet(GreetRequest) returns (GreetResponse) { }
}

The actual meaningful data would be stored as a simple JSON string, only using protocol buffers as a wire format. This is effectively JSON-over-RPC. Both the client and service would be responsible for serializing/deserializing the real data models to/from JSON. At this point, why even bother using protocol buffers?

The bottom-line

None of these are great options and are definitely against the general spirit of protocol buffers. The only way I can see a team using and actually benefiting from BXPB is if:

  1. They are developing a non-trivial extension which runs code in multiple contexts.
  2. They already use protocol buffers for most of their data models, and have existing organizational buy-in to do so.
  3. They are willing to put up with the current state of protobuf tooling.

I have a hard time seeing any team choose this state of affairs, they only find themselves in it due to other circumstances. The only viable use case I think of is Google itself, where:

  1. The prevalence of protocol buffers is significant enough that there is existing organizational buy-in to use protobufs for any kind of serialization or RPC problem.
  2. Most data models are already expressed as protocol buffers, and investing more in that area is a reasonable ask.
  3. Using Bazel, Closure, and owning the protobuf ecosystem (while sidestepping NPM and other bundlers) means the tooling is much more compatible and works out of the box in a reasonable way.

So now what?

So having now recognized this problem, what next? I could try another IDL which is more compatible with existing web tooling, but I'm not aware of any particularly good candidate. Even then, any other IDL will encounter the same problem of requiring a team to define their entire data model with it, which is just not how modern web development is done. REST APIs are typically done with JSON, not any special IDL format.

Accepting that any IDL format leads to the above problems (in some form or another), then to be effective we need to simplify the problem to avoid a dependency on the data format entirely.

The most immediate answer I see: let user-code serialize the data. If the API contracts simply treat request and response data as strings or simple (serializable) JavaScript objects, then defining a function looks something like:

// common.ts
export interface GreeterService {
  greet(request: string): Promise<string>;
}

export class GreetRequest {
  constructor(readonly name: string) { }

  serialize(): string {
    return JSON.stringify({ name: this.name });
  }

  static deserialize(serialized: string): GreetRequest {
    const json = JSON.parse(serialized);
    return new GreetRequest(json.name);
  }
}

export class GreetResponse {
  constructor(readonly message: string) { }

  serialize(): string {
    return JSON.stringify({ message: this.message });
  }

  static deserialize(serialized: string): GreetResponse {
    const json = JSON.parse(serialized);
    return new GreetResponse(json.message);
  }
}
// background.ts
serve<GreeterService>(chrome.runtime.onMessage, {
  async greet(request: string): Promise<string> {
    const req = GreetRequest.deserialize(request);
    const res = new GreetResponse({
      message: `Hello, ${request.name}!`,
    });
    return res.serialize();
  }
});
// popup.ts
const client = new Client<GreeterService>(chrome.runtime.sendMessage);
const req = new GreetRequest('Dave');
const serializedRes = await client.greet(req.serialize());
const res = GreetResponse.deserialize(serializedRes);
console.log(res.message); // Hello, Dave!

I'm taking some creative liberties with the implementations of serve<GreeterService> and Client<GreeterService>, these would likely need a value object for GreeterService (not just a type), or would require a compiler to generate implementations from a given TypeScript interface.

However, the main take away here is that a simple TypeScript interface is being used as the IDL format here. It has a few critical advantages:

  1. It plugs into existing data models. No matter what serialization method you use (JSON, protobufs, plain objects, etc.) it is all easily expressible with the interface.
  2. Web developers are already familiar with this, there is very little cognitive overhead to learn and use such a system.
  3. This does not require organizational buy-in, because all these tools and systems are already used by modern web developers!
  4. Adding serialize()/deserialize() functions is already a common pattern used when calling many REST APIs or storing data long term.
  5. Using generators/iterables/observables for streaming data fits nicely with the conceptual model.

There are also a few cons however:

  1. Users have to serialize/deserialize the data correctly themselves, there's nothing this library can do for them in that regard, largely by design.
  2. The types in the service interface still need to be serializable, which is not really intuitive with this developer experience. We'd need some compile-time or run-time check that the given object is serializable, and the interface would likely be defined with SerializedCar, SerializedPerson, and SerializedCompany types, rather than their "real" equivalents.
  3. Users must define their own serialize()/deserialize() functions.
  4. No existing "error" semantics. Will need to define our own error types, unlike protocol buffers which already have canonical errors.

This definitely requires some more thought and exploration to evaluate further and see if it has any merit.

So what happens to BXPB?

To be totally honest, as much fun as I had making this project, I don't see a valid path where it can become successful. I can see couple teams at Google potentially using it (after Bazel and third party integration, which is non-trivial amount of setup work before it becomes viable to individual projects), possibly one or two teams at other companies with similar tech stacks, and possibly a couple more teams using BXPB where they really shouldn't have and regretting it later. As a result, I can't justify additional work and effort going into the project at this point.

Things may change in the future. Maybe protobufs take off. Maybe they embrace the web ecosystem and become more usable and ergonomic. Maybe I'm overstating the perceived requirement of using protobufs throughout an application's entire data model.

As it stands, we're so close to a 1.0.0 release, that I think I want to push it over the finish line just to close this down. I'm thinking I'll wrap up the remaining tasks (largely by ignoring anything related to future work), release 1.0.0, and then deprecate it. That way passers-by can try it out, play with it, and learn from it. That will also leave the repo in a reasonable state if it ever gets revived or forked in the future. I think the historical and educational value of the project exceeds its practical application at this point, so simply preserving the current state is the best thing that can be done right now.

After that, I think I'll explore the viability of not using an IDL by letting user-code serialize the data. I think this has a lower potential than BXPB (as it is inherently less ergonomic by design, after all tooling is hypothetically perfected), but is much more achievable given the current state and culture of modern web development. I can only hope there is real potential in that direction.

dgp1130 commented 4 years ago

I published 1.0.0 of @bxpb/protoc-plugin and @bxpb/runtime to NPM. I then followed up with 1.0.1 with a simple change to the README displayed in NPM (which doesn't seem to have worked, but 🤷). I've also adding a comprehensive Getting Started doc which goes through setting up a simple service. It also links to some Git tags in the getting-started branch for users to reference.

Lastly I've updated the README and Wiki to indicate that the project is shut down, with a brief explanation of the decision and a link to the above comment which provides more thoughts on the matter.

I'm closing all the other open issues as obsolete (technically this one should be considered "Done", as I've accomplish all the requirements initially specified). I'll also archive the repository as it only exists for historical purposes now.

RIP BXPB.