Skip to content

Conversation

@Chriscbr
Copy link
Contributor

Fixes #717

The changes required to support this in the front-end have already been added in cdklabs/construct-hub-webapp#912.


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

@Chriscbr Chriscbr requested a review from a team March 12, 2022 00:01
@Chriscbr
Copy link
Contributor Author

Aside from the unit tests, I tested this by re-ingesting one package so far (and validated the multi-cdk tag now appeared on the front end) -- I'm now running a full reprocessing of the entire catalog to double check that there are no other issues.

Chriscbr and others added 2 commits March 11, 2022 20:10
Signed-off-by: github-actions <github-actions@github.com>
Comment on lines 14 to 17
it('classifies no major version if versions are mixed', () => {
expect(frameworkForDeps(['@aws-cdk/aws-s3@1.134.0', '@aws-cdk/aws-lambda@2.0.0'])).toEqual({ name: ConstructFrameworkName.AWS_CDK, majorVersion: undefined });
expect(frameworkForDeps(['@aws-cdk/aws-s3@1.134.0', '@aws-cdk/aws-lambda@2.0.0']))
.toEqual([{ name: ConstructFrameworkName.AWS_CDK, majorVersion: undefined }]);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it instead classify as BOTH?

Comment on lines +1 to +2
// eslint-disable-next-line @typescript-eslint/no-require-imports
import assert = require('assert');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For node/ESM related reasons I couldn't import this with either import { assert } from 'assert' or import assert from 'assert' without updating our esModuleInterop flag or something like that -- so I just had to import it like this. I don't think this is worth sweating over :P

Copy link
Contributor

Choose a reason for hiding this comment

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

I've usually been able to use import * as assert from 'assert'; without touching esModuleInterop.

@Chriscbr
Copy link
Contributor Author

Should it instead classify as BOTH?

This sounds interesting! - though I think providing extra information is a bit beyond the scope of this PR and the planned changes discussed in the original issue. 😅 For now, if a package happens to support multiple major versions, we'll still show the framework(s), just without the "v1" text, so our bases should be covered.

I don't think this needs to be flagged as a breaking change because the front-end code we are bundling this package with includes backwards-compatible logic for parsing either format produced by the backend. Though, it does raise the question of if we ever did make a backwards-incompatible change to the front-end, we don't currently have a mechanism to enforce a major version bump when that change is imported here (via the devDependencies), so... that's something to think about for the future.

@Chriscbr Chriscbr requested a review from RomainMuller March 14, 2022 22:53
@mergify mergify bot merged commit 78ea2dd into main Mar 15, 2022
@mergify mergify bot deleted the rybickic/multi-cdk branch March 15, 2022 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support multiple CDK badges per construct

2 participants