[
https://issues.apache.org/jira/browse/THRIFT-3122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14526011#comment-14526011
]
ASF GitHub Bot commented on THRIFT-3122:
----------------------------------------
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-98543767
> nice idea, but the introduction of new structs within
test/ThriftTest.thrift breaks many other language tests, do you really need new
structs? e.g. BonkMap is available as MapType
`MapType` is a typedef for `map<string,Bonk>`, `BonkMap` is a struct
containing member of type `map<string,Bonk>` - not the same thing.
The unit tests cover several basic cases:
- struct member is a struct (`Xtruct2` fits)
- struct member is a container that contains a struct
* list (`ListBonks` fits)
* set (no suitable existing type, `BonkSet` added)
* map (no suitable existing type, `BonkMap` added)
- struct member is a container that contains container (no suitable
existing type, `Bonkers` added - `CrazyNesting` doesn't fit because it has a
map with a struct as a key which can't work in Javascript and no values that
are structs)
It does appear that new structs are necessary. I understand that adding new
service methods would break other tests, but adding struct definitions
shouldn't break anything, it's really surprising (and wrong)
if it does. If that is indeed the case I can move new struct definitions to
a separate .thrift file to be used just in the deep constructor unit test.
> There is also a inconsistent naming: ListBonks, BonkSet, BonkMap
I can rename `BonkSet` -> `SetBonks`, `BonkMap` -> `MapBonks` to match
`ListBonks` which was there before, although if I need to move structs for the
deep constructor unit test to a separate file anyway I'd probably use different
names altogether. Let me know.
> Javascript struct constructor should properly initialize struct and container
> members from plain js arguments
> -------------------------------------------------------------------------------------------------------------
>
> Key: THRIFT-3122
> URL: https://issues.apache.org/jira/browse/THRIFT-3122
> Project: Thrift
> Issue Type: Improvement
> Components: JavaScript - Compiler
> Reporter: Igor Tkach
>
> Currently constructors for struct types in generated javascript accept
> {{args}} object and initialize struct's members by simply assigning a value
> from corresponding {{args}} object property (if not undefined). If struct
> member is
> another struct it must be explicitly created with constructor and passed as
> an argument value.
> Given following definitions:
> {code}
> struct A {
> 1: string something
> }
> struct B {
> 1: A value
> }
> {code}
> this works:
> {code:javascript}
> var b1 = new B(
> {
> value: new A(
> {
> something: 'hello'
> }
> )
> }
> );
> {code}
> this doesn't:
> {code:javascript}
> var b2 = new B(
> {
> value: {
> something: 'hello'
> }
> }
> );
> {code}
> Attempt to serialize b2 will result in error because {{b2.a}} doesn't have a
> {{write}} method.
> This becomes especially problematic when deep objects are used with libraries
> like [Underscore.js|http://underscorejs.org/], [lodash|https://lodash.com/],
> [React's immutability
> helpers|https://facebook.github.io/react/docs/update.html] or
> [Immutable.js|https://github.com/facebook/immutable-js]: most operations will
> return or produce plain javascript objects without read/write methods even if
> Thrift objects were given as input. Manually converting object graphs back to
> Thrift serializable form is not workable.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
