refactor: Add GuAppAwareConstruct mixin
#853
Conversation
akash1810
force-pushed
the
aa-mixin-app-aware
branch
2 times, most recently
from
7dfdd8d to
f748d64
Compare
| @@ -71,16 +72,16 @@ export class GuCertificate extends GuStatefulMigratableConstruct(Certificate) { | |||
| }) | |||
| ) | |||
| : undefined; | |||
| const awsCertificateProps: CertificateProps & GuMigratingResource = { | |||
| const awsCertificateProps: CertificateProps & GuMigratingResource & AppIdentity = { | |||
There was a problem hiding this comment.
Minor: could this be simplified to GuCertificatePropsWithApp & GuMigratingResource?
There was a problem hiding this comment.
Ah! I hadn't noticed that type. Unfortunately, the compiler isn't happy about this exact change as GuCertificatePropsWithApp wants some stage dependant values.
I've a suspicion we'll refactor how we do this once we address #850. That is, can I ask permission to be lazy? 😄
There was a problem hiding this comment.
Thanks for trying this out - my comment was a bit lazy to be honest, when I said "could this be simplified" it was because I genuinely wasn't sure if it'd build or not!
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- mixin | ||
| const [scope, id, props, ...rest] = args; | ||
|
|
||
| if (!AppIdentity.isAppIdentity(props)) { |
There was a problem hiding this comment.
I find this a little confusing... Presumably if you are trying to use the GuAppAwareConstruct Mixin but you haven't got an app property then you've made a mistake? I wonder if we should:
a) Investigate using Constrained Mixins to prevent people from making this mistake, or
b) Throw an error in this scenario
There was a problem hiding this comment.
Constrained mixins look a bit too advanced for me atm! I think we'd want to add a constraint based on the type/shape of the constructor? I find this variant a bit easier to read. That is, I'd prefer option b, but not sure if throwing is the right thing to do - is the intention that by throwing we'll catch issues at compile/test time rather than runtime?
There was a problem hiding this comment.
Constrained mixins look a bit too advanced for me atm! I think we'd want to add a constraint based on the type/shape of the constructor?
Yes I think so, but I was also struggling a bit with the docs so I'm happy to avoid this complexity for now!
is the intention that by throwing we'll catch issues at compile/test time rather than runtime?
Yes. For example, if we have code which starts using a field called idWithApp when we couldn't actually append an app name to the id, then this is likely to lead to confusing/buggy code, so I'd rather just fail with an error than let users get into this situation.
I don't feel too strongly about this though so feel free to merge as is if you're not keen on the idea of throwing
There was a problem hiding this comment.
I think throwing is sensible, especially as it a code path that shouldn't be taken. Will update.
There was a problem hiding this comment.
Done. Throwing is a lot clearer too! Thanks for the suggestion.
akash1810
force-pushed
the
aa-mixin-app-aware
branch
from
f748d64 to
ede8612
Compare
When a construct is app aware we:
- Suffix it's ID with the app string
- Add the `App` tag
Currently we have to remember to do this and remember it during review.
This change creates a mixin to simplify this.
Before
```typescript
interface MyBucketProps extends BucketProps, AppIdentity {}
export class MyBucket extends Bucket {
constructor(scope: GuStack, id: string, props: MyBucketProps) {
const idWithApp = AppIdentity.suffixText(props, id) # <-- this is getting replaced
super(scope, idWithApp, props);
AppIdentity.taggedConstruct(props, this); # <-- this is getting replaced
}
}
```
After
```typescript
interface MyBucketProps extends BucketProps, AppIdentity {}
class MyBucket extends GuAppAwareConstruct(Bucket) {
constructor(scope: GuStack, id: string, props: MyBucketProps) {
super(scope, id, props);
}
}
```
This is similar to #418.
akash1810
force-pushed
the
aa-mixin-app-aware
branch
from
ede8612 to
fb3be1c
Compare
|
🎉 This PR is included in version 27.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What does this change?
When a construct is app aware we:
ApptagCurrently we have to remember to do this and remember it during review.
This change creates a mixin to simplify this.
Before
After
In this change, we apply the new mixin to the
GuCertificateconstruct to demonstrate its usage.This is similar to #418.
Does this change require changes to existing projects or CDK CLI?
No.
Does this change require changes to the library documentation?
No.
How to test
Observe CI, it should continue to pass.
How can we measure success?
A DRYer codebase and less pressure on the review stage!
Have we considered potential risks?
As noted in #418: