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

fix(cli): remove source maps #32317

Merged
merged 4 commits into from
Nov 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions packages/aws-cdk/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -257,3 +257,20 @@ will change, and needs to be regenerated. For you, this means that:
2. When you build the CLI locally, you must ensure your dependencies are up to date by running `yarn install` inside the CLI package.
Otherwise, you might get an error like so: `aws-cdk: - [bundle/outdated-attributions] THIRD_PARTY_LICENSES is outdated (fixable)`.

## Source Maps

The source map handling is not entirely intuitive, so it bears some description here.

There are 2 steps to producing a CLI build:

- First we compile TypeScript to JavaScript. This step is configured to produce inline sourcemaps.
- Then we bundle JavaScript -> bundled JavaScript. This removes the inline
sourcemaps, and also is configured to *not* emit a fresh sourcemap file.

The upshot is that we don't vend a 30+MB sourcemap to customers that they have no use for,
and that we don't slow down Node loading those sourcemaps, while if we are locally developing
and testing the sourcemaps are still present and can be used.

During the CLI initialization, we always enable source map support: if we are developing
then source maps are present and can be used, while in a production build there will be no
source maps so there's nothing to load anyway.
5 changes: 2 additions & 3 deletions packages/aws-cdk/bin/cdk
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#!/usr/bin/env node

// source maps must be enabled before importing files
if (process.argv.includes('--debug')) {
process.setSourceMapsEnabled(true);
}
process.setSourceMapsEnabled(true);

require('./cdk.js');
6 changes: 5 additions & 1 deletion packages/aws-cdk/lib/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,7 @@ function determineHotswapMode(hotswap?: boolean, hotswapFallback?: boolean, watc
return hotswapMode;
}

/* istanbul ignore next: we never call this in unit tests */
export function cli(args: string[] = process.argv.slice(2)) {
exec(args)
.then(async (value) => {
Expand All @@ -568,7 +569,10 @@ export function cli(args: string[] = process.argv.slice(2)) {
})
.catch((err) => {
error(err.message);
if (err.stack) {

// Log the stack trace if we're on a developer workstation. Otherwise this will be into a minified
// file and the printed code line and stack trace are huge and useless.
if (err.stack && version.isDeveloperBuild()) {
debug(err.stack);
}
process.exitCode = 1;
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk/lib/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export function makeConfig(): CliConfig {
'ignore-errors': { type: 'boolean', default: false, desc: 'Ignores synthesis errors, which will likely produce an invalid output' },
'json': { type: 'boolean', alias: 'j', desc: 'Use JSON output instead of YAML when templates are printed to STDOUT', default: false },
'verbose': { type: 'boolean', alias: 'v', desc: 'Show debug logs (specify multiple times to increase verbosity)', default: false, count: true },
'debug': { type: 'boolean', desc: 'Enable emission of additional debugging information, such as creation stack traces of tokens', default: false },
'debug': { type: 'boolean', desc: 'Debug the CDK app. Log additional information during synthesis, such as creation stack traces of tokens (sets CDK_DEBUG, will slow down synthesis)', default: false },
'profile': { type: 'string', desc: 'Use the indicated AWS profile as the default environment', requiresArg: true },
'proxy': { type: 'string', desc: 'Use the indicated proxy. Will read from HTTPS_PROXY environment variable if not specified', requiresArg: true },
'ca-bundle-path': { type: 'string', desc: 'Path to CA certificate to use when validating HTTPS requests. Will read from AWS_CA_BUNDLE environment variable if not specified', requiresArg: true },
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk/lib/parse-command-line-arguments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export function parseCommandLineArguments(
})
.option('debug', {
type: 'boolean',
desc: 'Enable emission of additional debugging information, such as creation stack traces of tokens',
desc: 'Debug the CDK app. Log additional information during synthesis, such as creation stack traces of tokens (sets CDK_DEBUG, will slow down synthesis)',
default: false,
})
.option('profile', {
Expand Down
4 changes: 4 additions & 0 deletions packages/aws-cdk/lib/version.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ const UPGRADE_DOCUMENTATION_LINKS: Record<number, string> = {

export const DISPLAY_VERSION = `${versionNumber()} (build ${commit()})`;

export function isDeveloperBuild(): boolean {
return versionNumber() === '0.0.0';
}

export function versionNumber(): string {
// eslint-disable-next-line @typescript-eslint/no-require-imports
return require(path.join(rootDir(), 'package.json')).version.replace(/\+[0-9a-f]+$/, '');
Expand Down
1 change: 0 additions & 1 deletion packages/aws-cdk/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
"entryPoints": [
"lib/index.js"
],
"sourcemap": "linked",
"minifyWhitespace": true
}
},
Expand Down
12 changes: 11 additions & 1 deletion packages/aws-cdk/test/version.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import * as os from 'os';
import * as sinon from 'sinon';
import * as logging from '../lib/logging';
import * as npm from '../lib/util/npm';
import { latestVersionIfHigher, VersionCheckTTL, displayVersionMessage } from '../lib/version';
import { latestVersionIfHigher, VersionCheckTTL, displayVersionMessage, isDeveloperBuild } from '../lib/version';

jest.setTimeout(10_000);

Expand Down Expand Up @@ -141,3 +141,13 @@ describe('version message', () => {
expect(printSpy).not.toHaveBeenCalledWith(expect.stringContaining('Information about upgrading from 99.x to 100.x'));
});
});

test('isDeveloperBuild call does not throw an error', () => {
// To be frank: this is just to shut CodeCov up. It don't want to make an assertion
// that the value is `true` when running tests, because I won't want to make too
// many assumptions for no good reason.

isDeveloperBuild();

// THEN: should not explode
});
2 changes: 1 addition & 1 deletion tools/@aws-cdk/node-bundle/src/api/bundle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ export class Bundle {
bundle: true,
target: 'node14',
platform: 'node',
sourcemap: this.sourcemap ?? 'inline',
sourcemap: this.sourcemap,
metafile: true,
minify: this.minify,
minifyWhitespace: this.minifyWhitespace,
Expand Down