"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.

[eladb]

Okay, @rix0rrr was right. Having IXxx extend XxxAttributes doesn't really make sense for more complex resources that "bundle" other resources when they are exported.

For example, when you export a VPC, you export the set of public and private subnet IDs. This means that the output of export() looks like this:

  vpcId: string;
  availabilityZones: string[];
  publicSubnetIds?: string[];
  publicSubnetNames?: string[];
  privateSubnetIds?: string[];
  privateSubnetNames?: string[];
  isolatedSubnetIds?: string[];
  isolatedSubnetNames?: string[];

But in IVpcNetwork, these should be strongly typed:

  readonly vpcId: string;
  readonly publicSubnets: IVpcSubnet[];
  readonly privateSubnets: IVpcSubnet[];
  readonly isolatedSubnets: IVpcSubnet[];
  readonly availabilityZones: string[];

So, no! IVpcNetwork shouldn't extend VpcNetworkAttributes (or whatever we call it).

[eladb]

So, I think VpcNetworkAttributes doesn't make sense as the name for the output of export (and input of import).

Here's a merge between @rix0rrr's original idea and the fromXxx idea:

interface BucketExport {
  bucketArn: string;
  bucketName: string;
}

class Bucket {
  public static fromExport(scope: cdk.Construct, id: string, export: BucketExport): IBucket;
  public static fromBucketArn(scope: cdk.Construct, id: string, bucketArn: string): IBucket;
  public static fromBucketName(scope: cdk.Construct, id: string, bucketName: string): IBucket;
}

interface IBucket {
  bucketArn: string;

  export(): BucketExport;
}

Thoughts?

[eladb]

And we can create some awslint rules that will encourage/enforce fromXxx to be defined. For example, if there's a single attribute we can require that fromThatAttribute will be defined. If there is an ARN attribute, we can require that fromXxxArn will be defined, etc.

[sam-goodwin]

I think being explicit like this is right; it's intuitive/self-descriptive and has no chance of colliding with reserved words in any language.

How does fromExport and BucketExport look after cross-stack references are automated?

[rix0rrr]

I am okay with the proposed keywords; but I think our cross-stack export mechanism needs some thinking. We have to carry over a bunch of data, and there is no convenient serialization mechanism we can use

[rix0rrr]

I tend towards a structured set of exports keyed off an identifier string

[eladb]

That’s a good point Sam. I guess that we would only need “fromXxx” when cross stack references are implicit. @rix0rrr?

[eladb]

@sam-goodwin brought up a good point. We need to revisit imports/exports altogether given #1436. I will submit an PR with an RFC and will take the opportunity to resolve this as well.

[eladb]

Superseded by #2273