-
Notifications
You must be signed in to change notification settings - Fork 0
support for generating es6 imports and .d.ts files #1
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
base: main
Are you sure you want to change the base?
Conversation
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.
could you temporarily add the output of running this on test.proto so I can see the output?
example/test.d.ts
Outdated
@@ -0,0 +1,1013 @@ | |||
declare namespace proto.jspb.test { |
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.
We don't want namespaces in the es6 output. I think we can just remove this and unindent the contents.
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.
Removed. For nested messages and enums, I still use export namespace Foo { class Bar ... }
to represent Foo.Bar
, which as I understand it should merge with the sibling export class Foo { }
.
example/test.d.ts
Outdated
@@ -0,0 +1,1013 @@ | |||
declare namespace proto.jspb.test { | |||
export class Empty extends jspb.Message { |
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.
We need to import dependencies.
// @ts-nocheck | ||
|
||
import * as jspb from 'google-protobuf'; | ||
var goog = jspb; |
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 probably wiill need to be something like import { goog } from 'closure-library/goog.js
.
But that may be fine to figure out later.
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.
the correct import for goog is import * as goog from 'closure-library/closure/goog';
We still need to figure out how the jspb stuff should be imported.
jspb.Message.initialize(this, opt_data, 0, -1, null, null); | ||
}; | ||
goog.inherits(proto.jspb.test.MineField, jspb.Message); | ||
if (goog.DEBUG && !COMPILED) { |
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.
I wonder if we want something like this for es6 output
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.
Let's include it, added to the class object after definition, and declare it as | undefined
in the .d.ts.
* @param {boolean|undefined} includeInstance Deprecated. Whether to include | ||
* the JSPB instance for transitional soy proto support: | ||
* http://goto/soy-param-migration | ||
* @param {!proto.jspb.test.Empty} msg The msg instance to transform. |
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.
These types aren't right, they need to be imported
* @private {!Array<number>} | ||
* @const | ||
*/ | ||
static repeatedFields_ = [2]; |
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.
Closure compiler doesn't support static class properties yet. I think you need this after the class:
/**
* List of repeated fields within this message type.
* @private {!Array<number>}
* @const
*/
Simple1.repeatedFields = [2];
|
||
import * as jspb from 'google-protobuf'; | ||
var goog = jspb; | ||
var proto = {}; |
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 should go away in favor of imports (already done) and we should update all the types in the JSDoc strings to refer to the types we import.
Builds on protocolbuffers#150, adding:
library
option with ES6generate_dts
flag) and compatible (library
option set andimport_style
set toes6
)Tested compilation of Derivita's omaha/base/ast.proto and inspecting the generated
omaha/base/ast_proto.js
andomaha/base/ast_proto.d.ts
files.