Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider removing cloudformation namespace #878

Closed
ccurrie-amzn opened this issue Oct 9, 2018 · 18 comments · Fixed by #1311
Closed

Consider removing cloudformation namespace #878

ccurrie-amzn opened this issue Oct 9, 2018 · 18 comments · Fixed by #1311
Assignees
Labels
guidance Question that needs advice or information.

Comments

@ccurrie-amzn
Copy link
Contributor

ccurrie-amzn commented Oct 9, 2018

The cloudformation namespace creates some headaches when using ES6 imports. For example, if I need to import the L1 interfaces from two different modules (say, Lambda and API Gateway), I have to rename the namespaces so they don't conflict:

import { cloudformation as lambda } from "@aws-cdk/aws-lambda";
import { cloudformation as apig } from "@aws-cdk/aws-apigateway";

new lambda.FunctionResource();
new apig.RestApiResource();

or, alternatively rename the import, and use additional qualification.

import * as lambda from "@aws-cdk/aws-lambda";
import * as apig from "@aws-cdk/aws-apigateway";

new lambda.cloudformation.FunctionResource();
new apig.cloudformation.RestApiResource();

What I what I would like to do is import the L1 resources directly, but I cannot because of the namespace:

import { FunctionResource } from "@aws-cdk/aws-lambda";// Error:(3, 10) TS2305: Module '"@aws-cdk/aws-lambda/lib/index"' has no exported member 'FunctionResource'.

Typescript's manual discourages combining namespaces and modules.

@eladb
Copy link
Contributor

eladb commented Oct 10, 2018

We realized it's important to clearly separate between L1s and L2s and that's the reason we decided to namespace all L1 classes into a namespace.

I understand that this is not an ideal situation but, since we hope that people will be able to mostly use L2s, I wonder if "littering" the module's namespace is worth it.

@ccurrie-amzn
Copy link
Contributor Author

I understand the goals. As a side note, I think there will always be power users who want direct access to the L1s, and I wanted to propose something other than namespaces before the API got locked in a V1 release; IIUC they are a legacy of Typescript from before ES6 modules were standardized.

If you don't want the cloudformation types exported from the "root" module, perhaps the generated code could be placed in its own, "nested" module:

/
|-lib
  |- index.js
  |- implemenation_modules
|-cloudformation
  \- index.js

This would allow something like:

import { FunctionResource } from "@aws-cdk/aws-lambda/cloudformation";

It retains the value of keeping the root namespace clean while providing explicit, convenient access when needed. I don't know how this would play with JSII, however. Another option would be to break out the generated code into their own modules, but that is probably more headache for you than it would be a convenience for me.

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 11, 2018

You can actually do this today:

import { FunctionResource } from "@aws-cdk/aws-lambda/lib/lambda.generated";

It's not pretty but it works :)

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 11, 2018

And no, it does not translate across jsii, but that may not matter, since most languages have deep imports anyway?

// Java
import com.amazonaws.cdk.aws_lambda.cloudformation.FunctionResource;

// C#
using com.amazonaws.cdk.aws_lambda.cloudformation.FunctionResource;

// Python
from com.amazonaws.cdk.aws_lambda.cloudformation import FunctionResource

@ccurrie-amzn
Copy link
Contributor Author

You can actually do this today:

import { FunctionResource } from "@aws-cdk/aws-lambda/lib/lambda.generated";

True. I tend to view the contents of directories within modules as "private api" unless documented by the developer, preferring root-level imports.

[...] it does not translate across jsii, but that may not matter, since most languages have deep imports anyway?

That's an interesting point, and I suspect that JSII might have trouble with a module that publishes to both lib and cloudformation, so that option may not be available anyway.

FWIW, In the span of options I provided, when I'm a maintainer I actually prefer the "isolate generated code into separate modules" approach. I find it's much simpler to reason about a module's contents when it is either all generated or all hand written. But this particular issue isn't about maintenance but API surface, and I can of course achieve my goals through additional aliasing:

import { cloudformation as lambda } from "@aws-cdk/aws-lambda";
const FunctionResource = lambda.FunctionResource;

Or any similar variation. So import style is partly a matter of taste; I just find the namespace mechanism awkward.

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 16, 2018

I don't think we're going to change this, and the need for it is going to decrease as we're filling out or L2 library.

Feel free to reopen if you strongly disagree.

@rix0rrr rix0rrr closed this as completed Oct 16, 2018
@eladb
Copy link
Contributor

eladb commented Oct 24, 2018

I am reopening this...

There were a few conversations earlier around removing support for namespaces in jsii (since they are quite tricky to model in some programming languages). If we do end up removing support for namespaces, it means that we will need to find a solution for this.

We could consider extracting the L1 modules to a separate library (i.e. @aws-cdk/aws-s3-cloudformation or @aws-cdk/aws-s3-resources). The main motivation for merging L1 + L2 was because of attribute types (BucketArn). But now we almost got rid of those attribute types, and this motivation is gone.

We could also reconsider going back to the monolithic CFN package: @aws-cdk/cloudformation-resources...

Another option is to prefix all these types with something like CloudFormationBucketResource or even L1BucketResource.

@rix0rrr Thoughts?

@eladb eladb reopened this Oct 24, 2018
@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 24, 2018

My vote would be back to aws-cloudformation-resources I think. I like the layering clarity that comes from that, and I like that its presence makes our L2 libraries very much "nothing up our sleeves".

But we'll still need SOME kind of namespacing even then, for property types

@christophercurrie
Copy link

I imagine that it's simpler to maintain, but I would discourage a monolith; the aws-sdk for JS does this and it has some associated headaches, and I generally prefer a "don't pay for what you don't use" model.

@eladb
Copy link
Contributor

eladb commented Nov 24, 2018

Here are a few alternatives to consider. Let's add pros/cons to each (this is preliminary):

(1a) Just remove the namespace

Move the L1 classes (e.g. BucketResource) to the root namespace of the package. This means that we are going to have both Bucket and BucketResource side-by-side and the distinction between L1 and L2 will be based on the fact that there's a Resource suffix.

Pros:

  • Simple, flat, discoverable
  • Modular (each service owns it's own L1 resources)

Cons:

  • Could cause confusion: should I use Bucket or BucketResource?

(1b) Remove the namespace and rename the classes

Same as (1a) but also rename from BucketResource to CloudFormationBucket or CfnBucket. This might make it clearer what these classes are and will also free up the term "Resource" to represent the abstract concept of a cloud resource. The s3.Bucket class represents the S3 bucket resource (regardless that it's an L2).

(2) Another package per service

Move the generated classes to another package per service. So we'll have @aws-cdk/aws-s3 and @aws-cdk/s3-resources.

Pros:

  • Pure: better layering and distinction between generated L1s and L2s

Cons:

  • Proliferation of packages (we already have over 100 packages)
  • Built time
  • Could still be confusing

(3) A single package for all generated resources

See "cons" below, but this option will actually not eliminate the need for namespaces.

Pros:

  • Simple
  • Layering is clear (distinction between L1 and L2 is clearer)

Pro/Con:

  • This will eliminate any modules that do not have L2s - not sure if it's a pro or a con actually

Cons:

  • Everyone depends on this single package.
  • This single package will need to somehow namespace the classes in it, which means that we will need namespaces still.

@eladb
Copy link
Contributor

eladb commented Dec 5, 2018

I am leaning towards 1b - any other options/feedback/preferences?

@rix0rrr @RomainMuller

@RomainMuller
Copy link
Contributor

Leaning towards 1b as well, although the name addendum (prefix or suffix) maybe needs some thought.

Otherwise, I prefer 2 over 3 (everything in one bag is impractical), and 1 is a no-no from me (makes it hard for someone to know what class they should use when there is both L1 & L2 available).

@RomainMuller
Copy link
Contributor

Regarding 1b, after some off-line experimentation/discussion, it seems like using a Cfn (as an abbreviation for CloudFormation) as a prefix hits a sweet spot:

  • It doesn't look too odd
    • new lambda.CfnFunction(), as opposed to new lambda.$Function()
  • It's not too much extra typing for L1 user land
    • as opposed to new lambda.CloudFormationFunction()
  • It looks artificial enough to make it easy to tell what's L1 or L2
    • new lambda.Function() is L2
    • new lambda.CfnFunction() is L1
  • It interoperates well with case-conversion in JSII & doesn't use characters of dubious support in non-JS languages
    • snake_case(CFNFunction) is cfn_function
    • PascalCase(cfn_function) is CfnFunction

@AdamBoxall
Copy link
Contributor

AdamBoxall commented Dec 5, 2018

The problem with 3 is that it exposes the entire CloudFormation spec to users/IDEs, when most of the time they'll only need a small fraction of it. It may inadvertently encourage using L1 APIs, instead of installing the relevant L2 API, given it's already available in the project.

1b sounds like the safer option, but it's not very concise. @RomainMuller's suggestion I think is the best. The "Cfn" prefix is a bit ugly, but that should trigger to end users that's it not the right thing to be using.

@moofish32
Copy link
Contributor

I agree with most of the positive comments to 1b. I probably lean towards the longer name (1 it's clear and 2 I'm autocompleting anyway), but honestly either works for me.

Out of curiosity 2 initially annoys me, but if we separated this into @aws-cdk-cloudformation/aws-<service> and made this a separate scope can we eliminate the cons? The L1s change really on code generation and cfn spec, for the most part the number of devs interacting at that layer is pretty low. I don't think I want a separate repo, but we already trigger generation only once (i.e. if you are working in there you have to delete the file and trigger build). If the dev flow was use the released NPM version of the generated code the build time is gone. If you are developing on this section of code then run command x and lerna does the magic. Since, I don't know how many devs are really in that space of code my assumption above could be wildly wrong and void this proposal. There may also be massive problems in other maintainer flows that I don't know. An additional con is that you now have to require in the CloudFormation package if you want to use it and all the current scopes have to get the dependency added (which is 100 packages, but hopefully a script can do it). This might also mean you have to release this separately and that could double release efforts, again not sure on code churn here?

Worth any further investigation or did you guys consider this too and it didn't make the cut?

@eladb
Copy link
Contributor

eladb commented Dec 6, 2018

@aws-cdk-cloudformation/aws-

There is no technical difference between @aws-cdk/aws-<source>-resources and this option. I don't see how it can eliminate the cons really. Build time is still going to increase dramatically. Code gen is not the bottleneck, it's the TypeScript compilation, and mastering this in a separate repo is going to be challenging because the these classes depend on the CDK framework which is currently mastered in the same repo as the L2s, so we will need three repos (which is not such a bad idea going forward, but won't happen now).

There's another implication for this option, which is that it will basically mean that services that do not have L2s will have an empty library (with 0 exported types), or we will have to remove their library altogether. Some would say that's a good thing ("this service doesn't have L2s"), but I think in general, when someone needs a service, they would want a consistent experience, even if they would have to fall back to use L1s for that service.

@ccurrie-amzn
Copy link
Contributor Author

As an end user, (2) is still my preferred option, even with empty libraries, which I presume would still have a dependency on the cfn library. But I also understand that currently that imposes a high maintenance burden on you.

(1b) has an unstated con that upgrading users would have to rename all of their classes. Given that I make heavy use of L1s for services that have missing or incomplete L2s, (1a) would be my second choice of the options given.

@moofish32
Copy link
Contributor

@eladb -- I think the challenge for 2 is then this:

it's the TypeScript compilation, and mastering this in a separate repo is going to be challenging because the these classes depend on the CDK framework which is currently mastered in the same repo as the L2s, so we will need three repos (which is not such a bad idea going forward, but won't happen now).

I think you are saying that in order to really accomplish 2 we would need to do the following

  1. Separate the common core for CDK that both the L1 packages and L2+ packages would depend on (3 separate repos, but perhaps still in the mono-repo).
  2. Enable the ability to create separate releases for these components (big con for burden of work)
  3. Enable a new dev flow that is likely painful for anybody that needs develop across all three for a given change

Does that sound right? If so how much headache does 1b solve right now vs separating this repo later or now?

eladb pushed a commit that referenced this issue Dec 13, 2018
Rename generated CloudFormation resource constructs from `cloudformation.XxxResource` to `CfnXxx`. This fixes #878 and eliminates the use of namespaces in the CDK.

This is done in a backwards compatible way, which means that we still generate the old resources under the `cloudformation` namespace so we can remove them in a subsequent release. Those resources also include a deprecation warning which is emitted upon `cdk synth`.

Documentation updated to reflect changes.

Related: aws/jsii#283 and aws/jsii#270
@srchase srchase added guidance Question that needs advice or information. and removed question labels Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guidance Question that needs advice or information.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants