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

pbts does not generate code with correct protobufjs version #1228

Open
taylorcode opened this issue May 24, 2019 · 2 comments
Open

pbts does not generate code with correct protobufjs version #1228

taylorcode opened this issue May 24, 2019 · 2 comments
Labels

Comments

@taylorcode
Copy link
Collaborator

protobuf.js version: 6.8.8

Expected:
Using pbts with an input JS file that imports "protobufjs/minimal" should generate a .d.ts file with "protobufjs/minimal" imported.

Actual:
The .d.ts file imports from "protobufjs".

generated .js file

import * as $protobuf from "protobufjs/minimal";

generated .d.ts

import * as $protobuf from "protobufjs";
@alexander-fenster
Copy link
Contributor

Thanks for the report @taylorcode!

I'm pretty sure it's hardcoded here:

protobuf.js/cli/pbts.js

Lines 169 to 179 in f61b4bc

var imports = {
$protobuf: "protobufjs"
};
importArray.forEach(function(importItem) {
imports[getImportName(importItem)] = importItem;
});
// Write out the imports
Object.keys(imports).forEach(function(key) {
output.push("import * as " + key + " from \"" + imports[key] + "\";");
});

From what I see in pbts code, it does not actually parse the .js file but only uses jsdoc to take care of that. Not sure if the best idea is to add an option to pbts pointing to which library to use.

Given that it's just a .d.ts file so it's something that is only used during compile time, does importing protobufjs and not protobufjs/minimal breaks anything for you, or does it just generate extra stuff that does not present in minimal?

@taylorcode
Copy link
Collaborator Author

Thanks for your response Alexander. An option on pbts would work fine for my use case.

In the stack at my company, Dropbox, we do not use NPM/Yarn to install types. Types are vendored into the repo, so I was able to just create a protobufjs/index.d.ts file that re-exports the minimal types. This ensures that when developers import protobufjs/minimal, the types match what the generated code is using.

I imagine that most people do not encounter this issue, because they are using NPM/Yarn to install the types, and it is resolving the full-featured version of the types. I imagine this does not lead to any problems, since the generated code does not make use of features that are not in the minimal types.

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

No branches or pull requests

2 participants