fix(jsii): fail compilation when two or more enum members have same val

[yuth]

If two enum members have the same value, only the first one will be retained.

This is a bit of an issue as we are renaming enum members: the named version will never appear in the assembly, and so not work over jsii.

What's worse, if we deprecate-and-strip the original one, neither of the enum members will appear.

Addressing the issue by failing the compilation by adding validation for enum values

Related change in CDK aws/aws-cdk#19320
Fixes #2782.

---

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

[skinny85]

I don't think this change will work for CDK, where we use multi-value enums often (like here).

[RomainMuller]

I don't think this change will work for CDK, where we use multi-value enums often (like here).

I think the "right" way to fix this would actually be to have CDK use different values for each enum constant, and have code that gracefully normalizes downstream. That's however a little to easy to make mistakes with, and I wonder if we could come up with an eslint rule that makes this a little safer?

[skinny85]

I don't think this change will work for CDK, where we use multi-value enums often (like here).

I think the "right" way to fix this would actually be to have CDK use different values for each enum constant, and have code that gracefully normalizes downstream. That's however a little to easy to make mistakes with, and I wonder if we could come up with an eslint rule that makes this a little safer?

We can't do that, unfortunately, as that could be considered a breaking change.

[yuth]

I don't think this change will work for CDK, where we use multi-value enums often (like here).

I think the "right" way to fix this would actually be to have CDK use different values for each enum constant, and have code that gracefully normalizes downstream. That's however a little to easy to make mistakes with, and I wonder if we could come up with an eslint rule that makes this a little safer?

We can't do that, unfortunately, as that could be considered a breaking change.

My plan is to use the same value, but where it is duplicated, use a different value. So if a customer is using the string literal, it would still work as expected. And where we are using the enum transform them into the value needed by Cfn* construct

export enum SubnetType {
  ISOLATED = 'Deprecated_Isolated',
  PRIVATE_ISOLATED = 'Isolated',
  PRIVATE = 'Deprecated_Private',
  PRIVATE_WITH_NAT = 'Private',
  PUBLIC = 'Public'
}

and then update the instantiation code

// snip
    switch (subnetConfig.subnetType) {
        case SubnetType.PUBLIC:
          const publicSubnet = new PublicSubnet(this, name, subnetProps);
          this.publicSubnets.push(publicSubnet);
          subnet = publicSubnet;
          break;
        case SubnetType.PRIVATE_WITH_NAT:
        case SubnetType.PRIVATE:
          const privateSubnet = new PrivateSubnet(this, name, subnetProps);
          this.privateSubnets.push(privateSubnet);
          subnet = privateSubnet;
          break;
        case SubnetType.PRIVATE_ISOLATED:
        case SubnetType.ISOLATED:
          const isolatedSubnet = new PrivateSubnet(this, name, subnetProps);
          this.isolatedSubnets.push(isolatedSubnet);
          subnet = isolatedSubnet;
          break;
        default:
          throw new Error(`Unrecognized subnet type: ${subnetConfig.subnetType}`);
      }

[skinny85]

That's not a bad plan. Making sure @rix0rrr sees this (I remember him discussing this at lengths with Niranjan, although I can't find which issue that discussion was in).

[mergify]

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

[mergify]

Merging (with squash)...