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

feat: pass data types (structs) by-value instead of by-ref #376

Merged
merged 21 commits into from
Mar 20, 2019

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Mar 12, 2019

Data types ("structs") are defined through a TypeScript interface that only contains properties and doesn't begin with an I. This change also adds a requirement that all properties (shallow) are marked readonly.

This allows passing around structs by-value safely because any consuming code would not expect to be able to write to the object.

Python already passes structs by value, especially in the context of when a struct is used as keyword arguments. This use case will also exist in Ruby.

This is also the general perception around data that's passed around in most programming languages.

To enforce this, the jsii compiler will now fail if a struct includes properties that are not marked readonly.

Both the Java and .NET generators have been modified to generate builders which allow users to construct structs by serializing them as JSON hashes instead of passed down by reference.

Existing compliance tests in all languages have been fixed.

This change fixes aws/aws-cdk#965 and fixes #375 by erasing any nulls or undefined values from passed in objects, so they do not appear to have been defined at all. Added a compliance test called erase-unset-data-values to verify.

BREAKING CHANGE: all properties in interfaces which represent data types must be marked as readonly. Otherwise, jsii compilation will fail.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Elad Ben-Israel added 3 commits March 12, 2019 22:09
Implement a serialization method for Java POJOs (produced by
calling `build()` on the generated builders such that they
are passed by-value to JavaScript instead of by-reference.

Also, erase any nulls passed in objects to JS, so they are
treated as unset values for the purpose of `key in obj`.
This fixes aws/aws-cdk#965 and fixes #375.
@eladb eladb changed the title fix(java): pass java POJOs as values instead of as references fix(java): pass java POJOs by values instead of by reference Mar 13, 2019
Elad Ben-Israel added 7 commits March 13, 2019 15:23
…em matcher

Emit jsii diagnostics error when in watch mode, and also format the errors with a
"TS9999" error code so that VSCode's $tsc problem matcher will show them as
"Problems". Prefix "JSII" in the message to distinguish that these are jsii errors.

Fixes #382
@eladb eladb changed the title fix(java): pass java POJOs by values instead of by reference feat: pass data types by reference Mar 18, 2019
@eladb eladb requested a review from dstufft as a code owner March 18, 2019 23:03
@eladb eladb changed the title feat: pass data types by reference feat: pass data types by value Mar 18, 2019
@costleya
Copy link
Contributor

I'm trying to get a sense of what is different for .NET developers. You mention:

Both the Java and .NET generators have been modified to generate "builders" which allow users to construct these data types by serialize as JSON hashes instead of passed down by reference.

What does this look like? Are the conversions between .NET and JSII still transparent?

@eladb
Copy link
Contributor Author

eladb commented Mar 18, 2019

What does this look like? Are the conversions between .NET and JSII still transparent?

Yes, they are still transparent. The main change is that now when an object that implements a data type interface is passed down to jsii, it will be serialized as a json map instead of allocated as an anonymous reference.

@eladb
Copy link
Contributor Author

eladb commented Mar 19, 2019

Copy: @NetaNir

Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, what a change

@eladb
Copy link
Contributor Author

eladb commented Mar 19, 2019

That's going to be fun!

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 19, 2019

Any chance it's a small effort to update the C# generator to emit (an overload with?) keyword arguments?

And maybe null defaults?

I know it should be a different change, but while you're in here anyway...

@eladb eladb requested a review from garnaat March 19, 2019 11:06
@eladb
Copy link
Contributor Author

eladb commented Mar 19, 2019

Still waiting for an approval from:

@dstufft
Copy link
Contributor

dstufft commented Mar 19, 2019

That sounds fine to me!

@eladb eladb changed the title feat: pass data types by value feat: pass data types (structs) by-value instead of by-ref Mar 20, 2019
@eladb eladb merged commit db3ccdf into master Mar 20, 2019
@eladb eladb deleted the benisrae/pojos-by-value branch March 20, 2019 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants