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

feat(cli): bundle dependencies #18667

Merged
merged 82 commits into from
Feb 24, 2022
Merged

feat(cli): bundle dependencies #18667

merged 82 commits into from
Feb 24, 2022

Conversation

iliapolo
Copy link
Contributor

@iliapolo iliapolo commented Jan 26, 2022

Use esbuild via a custom new tool to bundle CLI dependencies and release a package with no runtime dependencies.

More details as to reasoning and implementation here.

Note

This PR has some implications on programmatic usage of the CLI.

Namely, deep imports like so:

import { PluginHost } from 'aws-cdk/lib/plugin'

Will no longer be available. These imports are considered private and should not have been used in the first place.
Instead, switch to:

import { PluginHost } from 'aws-cdk'

If your import isn't available from the top-level, it means that export is actually private, and should be avoided.


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

@gitpod-io
Copy link

gitpod-io bot commented Jan 26, 2022

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 26, 2022
@peterwoodworth peterwoodworth added the package/tools Related to AWS CDK Tools or CLI label Jan 26, 2022
Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed yesterday, there might be a middle ground here... We don't necessarily need to roll up the whole CLI, we could also only roll up each of the third party dependencies separately (i.e: esbuild the minimatch library to vendor/minimatch.js, etc...).

This is a little more work to get set-up, but would save you from having to deal with esModuleInterop stuff as well as import cycles.

It is also possible to configure esbuild to produce more than one entry point, which can also be used to make import cycles work better (although this is not a silver bullet).


Apparently, using import type for "type-only imports" (i.e: when you don't use the type's value, for instantiation, extension, instanceof, etc...) breaks the cycles (the imports/no-cycles rule will not consider statements that are not emitted as JS).

We should also frown on/prohibit the use of instanceof as it is very much a 🦶🏻🔫 in JavaScript. Although our dependency modeling TRIES to guarantee there is only ever ONE version of a given library... Sometimes npm does weird things... Using instanceof almost guarantees it won't work in that scenario, whereas using a dedicated API (i.e: symbol property check or something) could work (so this is a slightly better posture).

packages/aws-cdk/bin/cdk.ts Outdated Show resolved Hide resolved
packages/aws-cdk/lib/commands/context.ts Outdated Show resolved Hide resolved
packages/aws-cdk/lib/logging.ts Outdated Show resolved Hide resolved
packages/aws-cdk/lib/util/directories.ts Show resolved Hide resolved
packages/aws-cdk/package.json Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Feb 23, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Contributor

mergify bot commented Feb 23, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@iliapolo
Copy link
Contributor Author

@RomainMuller Anything else to add here?

@mergify
Copy link
Contributor

mergify bot commented Feb 23, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Contributor

mergify bot commented Feb 24, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@iliapolo iliapolo marked this pull request as ready for review February 24, 2022 09:00
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: a1c8a5f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Feb 24, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 31d135f into master Feb 24, 2022
@mergify mergify bot deleted the epolon/cli-bundle branch February 24, 2022 09:43
3p3r added a commit to 3p3r/cdk-web that referenced this pull request Feb 26, 2022
@stevehodgkiss
Copy link
Contributor

stevehodgkiss commented Mar 11, 2022

Unfortunately, this disables the use of aws-cdk in a pragmatic way (i.e. aws/aws-cdk-rfcs#300). We rely on invoking CDK programatically from a custom CLI library, so it looks like we'll be stuck on aws-cdk 2.13.0 until a solution is found. EDIT: Actually, this still appears to work, but dependencies of aws-cdk need to be manually pulled in to the custom CLI package (@aws-cdk/region-info, cdk-assets, proxy-agent etc), since they're bundled into aws-cdk/lib/index.js with this change as far as I can tell.

Following on from what @RomainMuller mentioned, is there a middle ground solution available (that doesn't disable programatic usage of aws-cdk)?

@stevehodgkiss
Copy link
Contributor

stevehodgkiss commented Mar 11, 2022

One approach might be to separate the bundled package (aws-cdk) from a package with the previous npm dependency management (aws-cdk-cli?). The code for aws-cdk would move to aws-cdk-cli, aws-cdk would depend on aws-cdk-cli and become a package to bundle it up with it's dependencies and CLI entrypoint. Normal/direct usage of aws-cdk would use the bundled package with strict control of dependencies, and pragmatic use of CDK could make use of aws-cdk-cli with normal npm dependency management. Thoughts?

Another approach would be to expose a supported programmatic API for CDK deployment functionality: aws/aws-cdk-rfcs#300

While it's still technically possible to use aws-cdk as a library, all of it's (now bundled) dependencies need to be defined on the package that uses it, since they're bundled into the aws-cdk entrypoint now and not defined as package dependencies. That's not ideal.

mergify bot pushed a commit that referenced this pull request Mar 17, 2022
Even since #18667, our build process validates that the `THIRD_PARTY_LICENSES` file of the CLI package is up to date. That is, the version of it committed to source code matches the one being auto generated.

This behavior breaks our automatic dependency upgrades whenever it includes an upgrade to one of the CLI's dependencies. This is because the autogenerated file will (for sure) have different dependency versions, and possibly also include new transitive dependencies. 

This manifests an error like so:

```console
aws-cdk: In package package.json
aws-cdk: - [bundle/outdated-attributions] THIRD_PARTY_LICENSES is outdated (fixable)
aws-cdk: Error: Some package.json files had errors
```

To fix this, we currently need to manually regenerate the `THIRD_PARTY_LICENSES` file by running `yarn pkglint` on the CLI package. 

> For example: 5ca8ebf

This PR adds a regeneration step to the upgrade workflow so that the PR also includes the up to date document. 
Note that if this doesn't mean attribution validation will always pass. If any dependencies changed licenses to one that isn't allowlisted, the validation will still fail.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mnapoli
Copy link
Contributor

mnapoli commented Mar 22, 2022

This is a huge BC break, it's very problematic for programmatic usage that was done of the CDK.

Would it be possible at least to export CloudFormationDeployments?

Currently aws-cdk exports the low-level deployStack function, but a lot of the CDK deployment logic is in CloudFormationDeployments.deploy (which calls deployStack).

We use the CDK programmatically in https://github.com/getlift/lift to integrate CDK with Serverless Framework (and we want to continue promoting CDK). I'm sure others have plenty of other use cases, as we can see by the number of upvotes in aws/aws-cdk-rfcs#300:

image

@Tanemahuta
Copy link

Tanemahuta commented Mar 22, 2022

The problem seems to be the lack of exposure for the libraries used by the cdk deployer.

But I found workaround.
The following example is working for version 2.17.0.

Please note: you need to install some dependencies:

npm i --save-dev @aws-cdk/cloud-assembly-schema@2.17.0 @aws-cdk/cloudformation-diff@2.17.0 @aws-cdk/cx-api@2.17.0

After that you'll be basically using the same approach, but "reloading" the assembly and

import * as cdk from 'aws-cdk-lib';
import {SdkProvider} from "aws-cdk/lib";
import {CloudFormationDeployments} from "aws-cdk/lib/api/cloudformation-deployments";
import * as cxapi from '@aws-cdk/cx-api';

const app = new cdk.App();
const stack = new Stack(app, "my-stack");

// Synthesize the app
const synthesized = app.synth();

// Reload the synthesized artifact for stack using the cxapi from dependencies
const assembly = new cxapi.CloudAssembly(synthesized.directory);
const stackArtifact = cxapi.CloudFormationStackArtifact.fromManifest(
    assembly, stack.artifactId,
    synthesized.getStackArtifact(stack.artifactId).manifest
) as cxapi.CloudFormationStackArtifact;

// Create a provider using set profile and use it for the CFN deployments service
const sdkProvider = await SdkProvider.withAwsCliCompatibleDefaults({profile: process.env.AWS_PROFILE});
const cfn = new CloudFormationDeployments({sdkProvider: sdkProvider});
// Deploy the stack
const deployResult = await cfn.deployStack({
    stack: stackArtifact, force: true,
});

Hope that helps.

mnapoli added a commit to serverless/compose that referenced this pull request Mar 22, 2022
mnapoli added a commit to serverless/compose that referenced this pull request Mar 22, 2022
@cgarvis
Copy link
Contributor

cgarvis commented Apr 13, 2022

@mnapoli sorry for the breaking change. The colors.js incident highlighted an attack vector for our customers that we need to close. With programmatic access being an unsupported use case, we hadn't considered it in our solution. We have a small team focused 100% on community PRs at the moment so the fastest way to unblock Lift would be to create a PR with your suggestions. I'll work from my side to get eyes on it.

For everyone else, messages and upvotes in closed PRs are hard for us to see. Please add your upvote to aws/aws-cdk-rfcs#300 to help us priortize.

codeyourfaceoff added a commit to codeyourfaceoff/cdk-ts-starter that referenced this pull request Apr 18, 2023
**Note:** due to [this pr](aws/aws-cdk#18667) we need to maually require the cdk's devDeps that we rely on to get programmatic access to it's apis. Not ideal but I understand where they're coming from
codeyourfaceoff added a commit to codeyourfaceoff/cdk-ts-starter that referenced this pull request Apr 18, 2023
**Note:** due to [this pr](aws/aws-cdk#18667) we need to maually require the cdk's devDeps that we rely on to get programmatic access to it's apis. Not ideal but I understand where they're coming from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. package/tools Related to AWS CDK Tools or CLI pr-linter/exempt-readme The PR linter will not require README changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants