-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add support for generating ES6 and TypeScript code #107
base: main
Are you sure you want to change the base?
Conversation
Maybe I'm missing something but this doesn't quite work for me. An empty message gives this:
There is a type error because |
Note, that is according to the |
There is a similar problem for
If only there were some type-safe (ish) language that PBF could have been written in to prevent these errors... :-P |
I think you are referring to the typings of I didn't change the internal implementation of You can test this by changing the generated signature and then run the tests. But I'm wondering why you get type errors. |
The problem is the I think the error might actually be in |
Apart from that type issue (which I may resolve by just uninstalling I have a 10 MB message that takes 10 seconds to decode with the official implementation, but with this it takes 170 millisconds! I thought Google engineers were meant to be good. The only thing I wish for was that it translated the type names from snake case to camel case properly. |
Yes, I think so, too. This is clearly a bug in |
Unfortunately if I remove
I tried creating How did you get this to work? |
Ah I found the magic "shut up typescript" incantation:
|
Ugh no that doesn't quite work because then you can't do |
Ok finally got it to work by copy/pasting the
|
Could you open a pull request with this change in |
Yep, though I'm not really sure about all the types. You might want to double check it yourself. E.g. are the
And is |
Ok I've been using this pull request for a little while and it seems to be working very well. My only suggestion is to add |
Actually there is one other issue - sub-messages. Consider:
This will give the following interfaces:
So far so good, however the read code sets
I'm pretty sure this is just a bug in pbf though; not in this change. This line should |
This fixes the type issues in mapbox#107. Also it makes more sense. I'm unsure about the `return undefined`.
No, I think it should be
|
14addeb adds that comment. |
Regarding Thanks for your pull request! |
* Use undefined instead of null as default value This fixes the type issues in #107. Also it makes more sense. I'm unsure about the `return undefined`. * Fix tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for working on this! 🎉 I'd love to land this. Do you plan on adding unit tests to cover the changes?
The change in the "normal mode" - I mean "each top-level message is generated into a single object literal" - is already covered by the existing tests. Testing the output of the new modes So I would propose to not add extra tests for this. |
Are there any updates on this? It would be a pretty nifty feature to have... |
Can you post the code snippet which is complained by TypeScript? Sure, you can send me a PR if you already have a fix. |
I'd love to see a release with this functionality. For tests, would it help to add set of tests asserting the expected compiled output for each format (js, es6, ts)? I don't see any tests like that for the standard js output, but with the addition of other formats, it might make sense? I'd be happy to add tests if it helps this PR to move forward :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After adding a pbf.d.ts / applying the other fix, I ran into two typescript issues.
I'll list them here as well as ideas on how to fix those.
for (var i = 0; i < fields.length; i++) { | ||
var field = fields[i]; | ||
var readCode = compileFieldRead(ctx, field); | ||
var packed = willSupportPacked(ctx, field); | ||
code += ' ' + (i ? 'else if' : 'if') + | ||
if (field.type === 'map' && !hasVarEntry) { | ||
code += ctx._indent + ' var entry'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will fail in case the proto is looking like:
int32 value = 1;
map<string, int32> dictionary = 2;
as it would produce something similar to:
if (tag === 1) ...
var entry: any
else if (tag === 2)
A quick way to solve it is to remove the hasVarEntry part and iterate the loop twice:
for (var i = 0; i < fields.length; i++) {
if (fields[i].type === 'map') {
code += ctx._indent + ' var entry';
if (options.moduleType === 'typescript') {
code += ': any';
}
code += ';\n';
}
}
function compileMapRead(readCode, name, numRepeated) { | ||
return (numRepeated ? '' : 'var ') + 'entry = ' + readCode + '; obj.' + name + '[entry.key] = entry.value'; | ||
function compileMapRead(readCode, name) { | ||
return 'entry = ' + readCode + '; obj.' + name + '[entry.key] = entry.value'; | ||
} | ||
|
||
function compileRepeatedWrite(ctx, field, numRepeated) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other typescript issue:
If I have a proto containing:
repeated customtype val = 1;
map<string, int32> map = 2;
I will get something like:
if (obj.val&& pbf) for (var i = 0;
...
if (obj.map && pbf) for (i in
...
The type of i is an number, but later it becomes a string.
A way to solve this is to do something like:
function compileRepeatedWrite(ctx, field, numRepeated) {
var iVal = 'i' + (numRepeated ? numRepeated.toString() : '');
return (
'for (var ' +
iVal +
' = 0; ' +
iVal +
' < obj.' +
field.name +
'.length; ' +
iVal +
'++) ' +
compileFieldWrite(ctx, field, 'obj.' + field.name + '[' + iVal + ']')
);
}
function compileMapWrite(ctx, field, numRepeated) {
var name = 'obj.' + field.name;
var iVal = 'i' + (numRepeated ? numRepeated.toString() : '');
return (
'for (var ' +
iVal +
' in ' +
name +
') if (Object.prototype.hasOwnProperty.call(' +
name +
', ' +
iVal +
')) ' +
compileFieldWrite(
ctx,
field,
'{ key: ' + iVal + ', value: ' + name + '[' + iVal + '] }'
)
);
}
@til-schneider @mourner What is the status here. I would love to this PR lands - I could help if something is missing or need to be updated. Please let me know. |
This whole project looks pretty dead to me. The last release was in Nov 2019. This pull requests is soon one year old and none of the project owners doesn't really care about. I can't judge the proposed changes of @UlysseM by simply looking at them. So I would have to create a test covering these cases. But since this pull request could have been merged already for months I don't see why it should be merged at any time. So I don't see why to invest any more efforts into this. For those who want to use this TypeScript support, you can use my forked repo in your "dependencies": {
"pdf": "github:junghans-schneider/pbf#master"
} |
Gosh. The project is very much alive and works perfectly for the purpose it was built for, and I could respond in detail on why this particular PR slipped through and what would have helped maintainers land this sooner, but with this kind of toxic attitude, I'm glad we never did. Have a nice day. |
I'm sorry. I really didn't want to insult you. I wanted to reply to @vicb's question "What is the status here". It was just my impression this project could be dead, because of the lack of any reaction for so many months. |
Actually, this would be very helpful. What is still missing from your perspective? |
I maintain and contribute to quite a few open source projects (including mapbox projects) and I know it is though as most people would only complain and be negative. However here people are proposing help and I can think that keeping this PR opened for so long without commenting on what need to be done is not less disrespectful than the comment from @til-schneider. To be clear I don't see either as toxic. Let's say that this PR has slipped but let's try to revive and merge this. I tend to agree with @til-schneider when it comes to mapbox repos. I use vector-tile-js, pbf, tippecanoe for one of my projects. Those packages are super helpful but sometimes it feels like they are not maintained. By no mean I want to be toxic by saying that but maybe mapbox should clarify what PR are accepted (bug fix only vs features, ...). If it doesn't fit with what people want to do with the code they can fork it. BTW my project also use togeojson but the tmcw fork because of nice fixes/evolutions he made. To come back on this CL: I don't think there is a need to generate ES5, ES6 and typescript. This makes the code more complex, more bug prone and harder to test. Generating only one version should be ok. I would favor generating typescript and let the TS compiler generate ESwhatever if needed. From the issue I created it seems like you don't agree. Then what about generating ES6 only and a .d.ts file for typescript ? (the source code need not be in typescript when there is a .d.ts). I think ES6 would allow for nice simplification of the code (i.e. reduce the line count, make it easier to understand). For example the context could use class inheritance instead of directly manipulating the (JS) protos. I hope we can move this forward together, (shameless plug: could yo chime in mapbox/vector-tile-js#74) |
Yes right, I don't agree. As said in #122, I think complexity would get up, not down. But I think we should not duplicate this discussion here. Let's keep the discussion in #122.
I don't see the point about the complexity of generating ES5, ES6 and TypeScript. This only affects the glue code around the actual logic (which stays the same for all flavors). I count 8 if statements in However, I do like the idea of generating a .d.ts file. Having a .d.ts would also help JavaScript users since many editors understand them and can provide assistance like code completion. But I see this as an extension which could be done after this pull request is merged. Therefore I think we should do this discussion in an extra issue. |
#122 has been closed by the maintainer.
Check @UlysseM comments on your CL. They all have subtle differences and you will eventually end up with 3 different code paths. While I'm sure you can handle that it will be a burden to fix bug and maintain this code - on top of having to write 3 times similar code and tests. There are very good tools to transpile the code today, no need to re-invent them in this repo in my opinion.
The idea would not be to ask user to transpile anything. The idea would still be that the compiler generates what ever output you tell him to generate but with only 2 lines of code, conceptually:
That would be transparent for the users and remove unneeded complexity here. The simpler the better - even more true for maintenance. |
I am very interested in ES6 output, not so much TypeScript. Maybe the tests suggested nearly a year ago (even if that adds a lot of dev dependencies) would help land this. |
@mourner: I already asked this a few months ago: What is still missing from your perspective? |
@til-schneider my philosophy for open source tools like this is to keep them minimal, simple, and as narrowly focused as possible, while being meticulously covered with tests, so that they can keep being useful for years without regular manual maintenance — that's the only way for me to manage maintaining 50+ projects at once, especially during challenging years like 2020. If we merge the PR in the current state, there will be no way to verify whether TypeScript or ES6 version continues to work after any subsequent PR, however small, without manually testing it. Same with any TS version upgrade. So covering with tests is essential, even if it means bringing in The other thing that I'm worried about is that nor I neither other maintainers are using TypeScript, so I don't feel qualified enough to decide in favor of a certain design decision (such as those people have disagreements about above) without spending a long time carefully considering each option, which I unfortunately don't have spare time for at the moment due to other commitments. Intuitively I like the option of providing |
@mourner: Very well explained! Thank you for this. I think it should be possible to test the ES6 version just with plain node (since node natively supports ES6 by now). What about this:
As already mentioned, the @mourner: Would you accept this PR with these changes? Maybe I should then drop the |
@til-schneider even if the PR isn't accepted, with those changes I'd probably start using your branch instead of the main project. It's pretty stable and shouldn't need much maintenance.
Yes 👍 |
I use https://github.com/timostamm/protobuf-ts which is a solid and well
maintained TS implementation. You might want to check it out.
…On Sun, Jan 3, 2021, 17:26 Andrew Herron ***@***.***> wrote:
@til-schneider <https://github.com/til-schneider> even if the PR isn't
accepted, with those changes I'd probably start using your branch instead
of the main project. It's pretty stable and shouldn't need much maintenance.
Maybe I should then drop the --typescript option and always generate a
.d.ts file in -es6 mode?
Yes 👍
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#107 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB4X4TWDNRL74EACLG2VSDSYEKLFANCNFSM4IQBXYJA>
.
|
Thanks but I don't actually need TS support, just ES6, and my aim is to use the library with the smallest bundle size. In my exploration |
That matches what I observed. Pbf is probably the best if bundle size is
your primarily concern.
…On Mon, Jan 4, 2021, 03:39 Andrew Herron ***@***.***> wrote:
I use https://github.com/timostamm/protobuf-ts which is a solid and well
maintained TS implementation. You might want to check it out.
Thanks but I don't actually need TS support, just ES6, and my aim is to
use the library with the smallest bundle size. In my exploration pbf was
easily the winner in that area.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#107 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB4X4RLQYBR3LYM4IU4TSLSYGSHVANCNFSM4IQBXYJA>
.
|
Before I do any more work on this PR, I need to know from @mourner at which point he would accept this PR. I could implement the suggestions from my last comment. But what would it be good for, if @mourner still didn't accept the PR. |
I'm also interested in this, to some extent. |
My two cents. This could be very helpful. The only reason we don't use pbf is bc to the lack of typescript definitions generation. |
This could also be very helpful for me as I'm trying to migrate my site to Typescript which depends on pbf. Will there be any progress on this PR? |
You can simply use |
Apologies for commenting on an old neglected PR. My last few years haven't been easy to put it mildly — I'm a Ukrainian living in Kyiv, there's only so much energy to work on non-critical open source maintenance. I finally just got my hands on overhauling and modernizing the project, adding ES codegen by default, first-class TS types for the package itself, updating dev tools and code practices, and greatly extending test coverage — check out #134. I'll keep this PR open for reference when we're ready to add TS codegen support. |
This adds two options
--es6
and--typescript
which allow to generate ES6 or TypeScript code.Example ES6 output:
Example TypeScript output:
Further more each top-level message is generated into a single object literal:
before:
after: