chore(cli): show warning when using deprecated version #102

tttol · 2025-02-23T14:02:19Z

What I changed

Show the warning message when Toolkit uses deprecated version of aws-cdk. The message is like below.

You are using deprecated version(aws-cdk@X.X.X): This version has been deprecated

Additional information

I'm new to aws-cdk.
I didn't submit PR to aws-cdk-cli-testing because I think that integration test is not needed in my change. But I am not confident in my judgment. Please let me know if additional integration tests are needed.

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

rix0rrr · 2025-02-25T08:57:32Z

packages/aws-cdk/lib/cli/util/npm.ts

+
+export async function checkIfDeprecated(version: string): Promise<string | null> {
+  try {
+    const { stdout, stderr } = await exec(`npm view aws-cdk@${version} deprecated --silent`, { timeout: 3000 });


Can you work this into the existing version check in some way? (it may be that you will need to change that existing check to return information about more than 1 version at a time).

I'd like to avoid doing unnecessary calls to NPM.

@rix0rrr
It's true that my implementation results in unnecessary NPM calls. But I couldn't find the logic of version check in existing source code. Which logic/file are you referring when you said work this into the existing version check?

We're doing upgrade notice work here already: https://github.com/aws/aws-cdk-cli/blob/huijbers/install-deps/packages/aws-cdk/lib/cli/version.ts/#L86

@rix0rrr
Do you mean that calling checkIfDeprecated out of if (laterVersion) block is better?
Ref: https://github.com/aws/aws-cdk-cli/blob/main/packages/aws-cdk/lib/cli/version.ts#L125

try { const laterVersion = await latestVersionIfHigher(currentVersion, versionCheckCache ?? new VersionCheckTTL()); + const deprecationMessage = await checkIfDeprecated(currentVersion); + if (deprecationMessage) { + info(`You are using deprecated version(aws-cdk@${currentVersion}): ${deprecationMessage}`); + } if (laterVersion) { const versionMessage = await getVersionMessage(currentVersion, laterVersion);

I mean I don't want 2 calls to npm view. I would like a single call to npm view to return both bits of information:

What is the latest version?

Is the current version deprecated?

It might be that that is not possible, but I would like that to be researched and confirmed first.

It's somewhat possible to get this with a single request:

https://registry.npmjs.com/aws-cdk/

The downside is that this is a very large response object. Individual versions or latest can be fetched as well:

https://registry.npmjs.com/aws-cdk/2.1003.0

https://registry.npmjs.com/aws-cdk/latest

It might be prudent to change this feature over to use https calls anyway, to be less reliant on the npm binary. If we download the huge file, we definitely need to cache the result. Making two smaller https requests in parallel migh be faster, not sure. Probably still want to have a cache of sort or at least ensure this isn't checked all the time.

@rix0rrr @mrgrain
Thank you for your advice. I will try to retrieve the latest version and deprecation message in a single npm viewcall.

$ npm view aws-cdk@1.204.0 deprecated name --json { "deprecated": "AWS CDK v1 has reached End-of-Support on 2023-06-01.\nThis package is no longer being updated, and users should migrate to AWS CDK v2.\n\nFor more information on how to migrate, see https://docs.aws.amazon.com/cdk/v2/guide/migrating-v2.html", "name": "aws-cdk" }

Hm. I don't think this single npm view call will be able to retrieve both bits of information.

I guess the best solution would be then to do 2 parallel calls to npm view for both bits of information, and run them in parallel with await Promise.all().

Heyyy that's what you did 😆

- Refactored version checking logic to use `execNpmView` for retrieving package metadata - Updated version message generation to handle deprecated versions and upgrade recommendations - Simplified npm utility functions to return parsed JSON from npm view command - Updated tests to mock new npm view behavior and improve test coverage

rix0rrr · 2025-03-11T18:21:17Z

packages/aws-cdk/lib/cli/util/npm.ts

+      .catch(err => {
+        throw new ToolkitError(`Failed to fetch latest version info: ${err.message}`);
+      }),


I'm not super up on Promises syntax. Is catch with a throw inside the right way to do this?

Where are these errors handled?

Is catch with a throw inside the right way to do this?

It’s not the wrong way in TypeScript, but this implementation is a little confusing. I used a single catch to handle all thrown exceptions.

rix0rrr · 2025-03-11T18:22:07Z

packages/aws-cdk/lib/cli/version.ts

-  const latestVersion = await getLatestVersionFromNpm();
-  const isNewer = semver.gt(latestVersion, currentVersion);
+  const packageInfo = await execNpmView(currentVersion);
+  const latestVersion = packageInfo.latestVersion;
  await cacheFile.update(latestVersion);


I think we should probably stick both fields into the cachefile? Both latestVersion and deprecated?

The entire packageInfo object therefore.

I changed the logic to cache the entire packageInfo.

rix0rrr · 2025-03-11T18:23:31Z

packages/aws-cdk/lib/cli/version.ts

+  if (semver.eq(latestVersion, currentVersion)) {
+    return [];
  }


This is only correct if you assume the latest pointer never points to a deprecated version. If that's something you assume, you should put it in a comment.

I added the comment 👍

rix0rrr · 2025-03-11T18:25:22Z

packages/aws-cdk/lib/cli/version.ts

  }
+
+  const versionMessage = [
+    `${chalk.red(packageInfo.deprecated as string)}`,


Will this string ever be empty so that the filter below removes it? I have a feeling it would contain ANSI escape characters without characters in between? Have you tested?

Exactly. I changed it to check if packageInfo.deprecated is undefined before calling chalk.red.

tttol added 3 commits February 23, 2025 07:02

add logic and new test

b6331f5

fix test

a56d547

fix test

bf4a6a3

tttol requested a deployment to integ-approval February 23, 2025 14:02 — with GitHub Actions Waiting

aws-cdk-automation enabled auto-merge February 23, 2025 14:02

tttol changed the title ~~chore(cli):show warning when using deprecated version~~ chore(cli): show warning when using deprecated version Feb 23, 2025

rix0rrr requested changes Feb 25, 2025

View reviewed changes

tttol requested a review from rix0rrr March 2, 2025 21:10

auto-merge was automatically disabled March 10, 2025 20:16
Head branch was pushed to by a user without write access

tttol requested a deployment to integ-approval March 10, 2025 20:16 — with GitHub Actions Waiting

Add timeout to npm view command

6672b83

tttol requested a deployment to integ-approval March 10, 2025 20:20 — with GitHub Actions Waiting

Pass current version to npm view command

64e08b5

tttol requested a deployment to integ-approval March 10, 2025 20:27 — with GitHub Actions Waiting

Merge branch 'main' into show-deprecation-msg

ca2de44

tttol requested a deployment to integ-approval March 10, 2025 20:34 — with GitHub Actions Waiting

tttol marked this pull request as draft March 10, 2025 20:39

exec two npm call in parallel

3c2979f

tttol requested a deployment to integ-approval March 10, 2025 23:16 — with GitHub Actions Waiting

rix0rrr reviewed Mar 11, 2025

View reviewed changes

Fixed issues from PR feedback

381c90e

tttol requested a deployment to integ-approval March 11, 2025 22:11 — with GitHub Actions Waiting

tttol marked this pull request as ready for review March 11, 2025 22:26

aws-cdk-automation enabled auto-merge March 11, 2025 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(cli): show warning when using deprecated version #102

chore(cli): show warning when using deprecated version #102

tttol commented Feb 23, 2025 •

edited

Loading

rix0rrr Feb 25, 2025

tttol Feb 25, 2025 •

edited

Loading

rix0rrr Mar 3, 2025

tttol Mar 3, 2025 •

edited

Loading

rix0rrr Mar 4, 2025

mrgrain Mar 7, 2025

tttol Mar 7, 2025

rix0rrr Mar 11, 2025

rix0rrr Mar 11, 2025

rix0rrr Mar 11, 2025

tttol Mar 11, 2025 •

edited

Loading

rix0rrr Mar 11, 2025

tttol Mar 11, 2025

rix0rrr Mar 11, 2025

tttol Mar 11, 2025

rix0rrr Mar 11, 2025

tttol Mar 11, 2025

chore(cli): show warning when using deprecated version #102

Are you sure you want to change the base?

chore(cli): show warning when using deprecated version #102

Conversation

tttol commented Feb 23, 2025 • edited Loading

What I changed

Additional information

Choose a reason for hiding this comment

tttol Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tttol Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tttol Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tttol commented Feb 23, 2025 •

edited

Loading

tttol Feb 25, 2025 •

edited

Loading

tttol Mar 3, 2025 •

edited

Loading

tttol Mar 11, 2025 •

edited

Loading