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(.net): embed package icon when configured #3676

Merged
merged 6 commits into from
Aug 4, 2022

Conversation

RomainMuller
Copy link
Contributor

When an iconUrl is configured for a .NET target, attempt to download
it for inclusion in the NuGet package with the PackageIcon attribute,
as the PackageIconUrl attribute is deprecated.

This feature can be opted out of by setting the
JSII_PACMAK_DOTNET_NO_DOWNLOAD_ICON environment variable (no matter
what value it has).

If the download somehow fails, the previous behavior is preserved.

The PackageIconUrl attribute is still emitted for backwards
compatibility with tools that do not (yet) support PackageIcon.


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

When an `iconUrl` is configured for a .NET target, attempt to download
it for inclusion in the NuGet package with the `PackageIcon` attribute,
as the `PackageIconUrl` attribute is deprecated.

This feature can be opted out of by setting the
`JSII_PACMAK_DOTNET_NO_DOWNLOAD_ICON` environment variable (no matter
what value it has).

If the download somehow fails, the previous behavior is preserved.

The `PackageIconUrl` attribute is still emitted for backwards
compatibility with tools that do not (yet) support `PackageIcon`.
@RomainMuller RomainMuller requested a review from a team July 25, 2022 14:26
@RomainMuller RomainMuller self-assigned this Jul 25, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 25, 2022
Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

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

Just a few comments / questions

}
break;
default:
// Nothing to do...
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's not one of these, how can it be a valid icon? Shouldn't this be an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec unfortunately does not specify what is a valid MIME type for the icon, and there are more image formats that I can't be bothered with (ico, heic, etc..)

// Attempt to download the package icon from the configured URL so we can use the non-deprecated PackageIcon
// attribute. If this fails or is opted out (via $JSII_PACMAK_DOTNET_NO_DOWNLOAD_ICON being set), then only the
// deprecated PackageIconUrl will be emitted.
const iconFile =
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way for the user to pass this file without downloading it (eg if they already have the icon locally)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not currently... That'd require they bundle the icon in their npm package, which I'm not sure is appropriate...

Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

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

LGTM!

@mergify
Copy link
Contributor

mergify bot commented Aug 3, 2022

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Aug 3, 2022
@mergify
Copy link
Contributor

mergify bot commented Aug 3, 2022

Merging (with squash)...

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Aug 3, 2022

Merging (with squash)...

…e-icon

# Conflicts:
#	packages/jsii-calc/test/assembly.jsii
@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2022

Merging (with squash)...

@mergify mergify bot merged commit c65b1d9 into main Aug 4, 2022
@mergify mergify bot deleted the rmuller/dotnet-package-icon branch August 4, 2022 09:13
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Aug 4, 2022
mrgrain added a commit to projen/projen that referenced this pull request Aug 16, 2022
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants