"import" is a reserved word in Java

[eladb]

All AWS constructs have a static import method to allow bringing-in resources defined externally. However, import is reserved in Java (and possibly in other languages).

In the interim the jsii compiler will simply add an _ at the end of the method name, but we need to find a new name for the method and rename across the codebase.

[rix0rrr]

I think we shouldn't need to export/import anyway, see cross-stack references design

[eladb]

That's one way to address this problem 😄

[eladb]

That's something we should probably do before 1.0

[rix0rrr]

Yeah we do need to export()/import() for cross-app references.

[eladb]

Also, we should standardize the naming for importFromContext (which I argued should be importFromEnvironment).

[eladb]

Okay, here's a proposal (following an online discussion with @RomainMuller and @rix0rrr). We'll use Bucket as an example:

interface BucketAttributes {
  readonly bucketArn: string;
  readonly bucketName: string;
}

// mixing data types and interfaces?? would that be okay?
interface IBucket extends BucketAttributes {
  export(): BucketAttributes;
}

class Bucket {
  public static fromAttributes(parent: Construct, id: string, attributes: BucketAttributes): IBucket;

  // if you only have a bucket ARN we can provide a convenience method which parses the
  // bucket name from the ARN and vice versa
  public static fromArn(parent: Construct, id: string, bucketArn: string): IBucket;
  public static fromBucketName(parent: Construct, id: string, bucketName: string): IBucket;
}

The only problem here is that we are mixing interfaces and data types (IBucket extends BucketAttributes). I am not 100% sure that's an issue (see aws/jsii#271, aws/jsii#265). @rix0rrr thoughts?

[rix0rrr]

I don't think the interface inheritance is going to be an issue.

On the other hand, not convinced the combination of FooAttributes Foo.fromAttributes() and IFoo extends FooAttributes is going to work for more than the trivial resource types.

Also seems like the fromArn and fromBucketName could have been subsumed by fromAttributes() with optional parameters and runtime checking. We do this in many other cases, I'm not sure why this warrants top-level functions all of a sudden.

All's I'm saying is that I don't think we gain anything by tying FooAttributes so tightly to IFoo, except limiting our own freedom of movement.

[eladb]

I am feeling uneasy about the runtime checks we have around imports. It’s a bit of ugliness I’d love to get rid of. It also occasionally violate the rule that says that if all properties are optional then “props” should also be optional.

The reason I feel this tie in makes sense is because there is a strong correlation between “import props” (for lack of a better term at the moment) and the set of attributes intrinsically available for a resource. I don’t like the fact that today we literally have to duplicate this set between BucketImportProps and IBucket, only with different levels of optionality.

The “fromAttributes” method (with everything non-optional) will be useful espeically for exports and the various fromXxx methods will be more useful when you have a specific attribute value such as an ARN, etc. I like the fact that this approach gives us the ability to express the various import use cases without sacrificing type safety (contrary to the runtime checks approach).

[RomainMuller]

I agree with @eladb's point on the optionals issue. Since JSII can't allow "all type unions", the separate methods are the only way to restore type safety while allowing possibly disjoint sets of required/optional fields.

[rix0rrr]

I don’t like the fact that today we literally have to duplicate this set between BucketImportProps and IBucket, only with different levels of optionality.

The argument against DRY is that some things may look the same in some cases, but they're actually not the same, and now you've introduced coupling that shouldn't be there.

I think S3 is a poor use case here. If you can make this design work in the ELBv2 package and/or the ECS package, then I withdraw my objections.

[eladb]

@rix0rrr fair enough. Will definitely try to see how it plays out in those cases.