voho
commented
1 year ago Hi! Thanks for all the tips. There has been a lot of history to this library, which is probably the cause of the current state. As you say, fixing these would mostly result in incompatible changes, which we try to avoid and/or group together as much as possible. We could create some special label for these and pick them up with the next major version upgrade, and split them into actionable chunks.
Feature scope
core
Describe your suggested feature
This library looks useful, but it doesn't seem to fit with some CDK style and conventions. Some design choices reveal Java influence. Clashing with CDK style/conventions is a warning sign for CDK users that could discourage adoption.
Fixing these issues would require big design changes. Hopefully this can be useful as a customer experience report for future directions.
Exposing Design Pattern Language
This library exposes the domain of design patterns to its users: Facade, Strategy, Factory.
While common in some Java libraries, naming classes based on design patterns is rare in TypeScript. Internally, using these terms could be useful (CDK often uses "Factory" internally) but avoiding them in the user interface would make the library seem more idiomatic.
User-Facing Fluent APIs
I like fluent APIs but historically they've been avoided or even removed in CDK, being described as "not idiomatic to the CDK".
Nested MonitoringFacadeProps
The nested MonitoringFacadeProps are unusual for CDK. The construct design guidelines discourage nesting for construct props. This is probably not a case where simply adding a prefix to flatten the props would yield a satisfying result. This class is a factory for factories (.createAlarmFactory), a dashboard (.addSmallHeader), and the interface for creating all resource-specific monitoring constructs (.monitorSqsQueue). Probably changing the overall fluent api approach would remove the need for these nested props.