-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[TS] Fix generation of struct members in object api #7148
Conversation
Pull from google/flatbuffers
If a struct has a key the vector has to be sorted. To sort the vector you can't use "const".
* option indent_step is supported
Hello, I'm not the TS maintainer but I am working on standardizing our code generation and would like you opinion on #7142. I'm curious why "field accessor function must be unescaped" does TS/JS first resolve fields before trying to find keywords? I noticed you only changed Also, pull again - I managed to break master when adding a printf and I fixed that. |
About reserved word as functions in TS. Yes - its a bit strange, but indeed reserved words are allowed as function/method names. In other languages that's not the case. So TS seams to be a special snowflake here. But in my opinion its more convenient for a user of TS to not escape these method names. I will add a test case to the fbs file and try to run the test. |
@krojew can we get a review? |
(this.b === null ? 0 : this.b.b!), | ||
(this.c === null ? 0 : this.c.id!), | ||
(this.c === null ? 0 : this.c.distance!) | ||
(this.a === null ? 0 : this.a?.id!), |
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 seems quite odd - you first allow a
to be undefined (?), but then you assert the value is present (!). Looks contradictory. The same pattern repeats for other fields in this PR.
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.
Of cause this looks a bit strange. We could simplify this line from (this.a === null ? 0 : this.a?.id!)
to (this.a?.id!)
. It is equivalent and should compile.
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 would still be wrong - the result of the ?.
operator can be undefined, so you can't later assert it's not by using !
.
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.
What would you suggest?
Would this solution be ok? :
- struct of struct:
(this.a === null ? 0 : this.a.id!)
- struct of struct of struct:
(this.a?.b === null ? 0 : this.a.b.id!)
<- i think this will not compile
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.
Why do you need ?.
in the first place? There is already a null check present.
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.
Ok, I see what the problem might be for nested structs, but the proposed code doesn't solve it. For example, this.a?.a === null
will never be true if this.a
is null, thus defeating the check. Instead, something like this should work: this.a?.a ?? 0
.
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.
You are right. The solution is:
- struct of struct:
this.a?.id ?? <nullvalue>
(wrong:this.a?.id && <nullvalue>
) - struct of struct of struct:
this.a?.b?.id ?? <nullvalue>
(wrong:this.a?.b?.id && <nullvalue>
=
is false
for type bool and 0
for all other simple data types.
I will try it out. If it works i will alter this pull request. I will also write a test case for this.
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.
No, this is not what I proposed - using &&
instead of ??
will not work.
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.
Of cause - a typo at my side. Thanks
Are we all good to go on this? |
From my point of view yes. |
Thanks as always! |
If a struct is used in a struct TS generates wrong code.
Example:
struct A {
testBool: bool;
}
struct B {
teststruct: A;
}
Furthermore: