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(core): show warnings on unused / redundant package extensions #2091

Merged
merged 15 commits into from
Nov 17, 2020

Conversation

paul-soporan
Copy link
Member

@paul-soporan paul-soporan commented Nov 6, 2020

What's the problem this PR addresses?

Package extensions weren't validated in any way, which meant that they could easily get out of sync with the installed dependency tree without anybody noticing.

How did you fix it?

I've made it so that we emit warnings at the end of the resolution step (we can only know which package extensions are used once we resolve all packages) on:

  • unused package extensions - happens when a package extension selector doesn't match any installed package
  • redundant package extensions - happens when things would have the same behavior both with and without the extension

I've also fixed a weird bug with boolean values being loaded as strings from the configuration files and the lockfile - I've ranted enough about it on Discord where you can find more details.

You can see how it looks in this poorly-cropped screenshot below:

image

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@paul-soporan paul-soporan requested a review from arcanis as a code owner November 6, 2020 22:38
.yarnrc.yml Outdated Show resolved Hide resolved
@@ -1579,8 +1599,8 @@ export class Project {
for (const extensionsByIdent of this.configuration.packageExtensions.values())
for (const [, extensionsByRange] of extensionsByIdent)
for (const extension of extensionsByRange)
if (extension.active)
Configuration.telemetry?.reportPackageExtension(extension.description);
if (extension.userProvided && extension.status === `active`)
Copy link
Member Author

Choose a reason for hiding this comment

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

I think it makes more sense to only report user-provided and non-redundant extensions inside our telemetry collection.

Copy link
Member

Choose a reason for hiding this comment

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

I think it might actually make a lot of sense to report the redundant builtins, since it means we can remove them during the next major. Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a few concerns:

  • We can't tell which builtins are ours and which are registered by custom (possibly private) plugins. We can only distinguish between user-provided and not user-provided.
  • Redundant builtins are relative to a project and its installed dependency tree, since a builtin can match multiple packages and can be active for some of them and redundant for others (e.g. if a dependency issue gets fixed in a newer version of a package).
  • We'd have to report redundant builtins separately to be able to distinguish them inside the dashboard from active user-provided extensions.

@@ -1579,8 +1599,8 @@ export class Project {
for (const extensionsByIdent of this.configuration.packageExtensions.values())
for (const [, extensionsByRange] of extensionsByIdent)
for (const extension of extensionsByRange)
if (extension.active)
Configuration.telemetry?.reportPackageExtension(extension.description);
if (extension.userProvided && extension.status === `active`)
Copy link
Member

Choose a reason for hiding this comment

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

I think it might actually make a lot of sense to report the redundant builtins, since it means we can remove them during the next major. Wdyt?

packages/yarnpkg-core/sources/Configuration.ts Outdated Show resolved Hide resolved
packages/yarnpkg-core/sources/Configuration.ts Outdated Show resolved Hide resolved
packages/yarnpkg-core/sources/Manifest.ts Outdated Show resolved Hide resolved
packages/yarnpkg-core/sources/Manifest.ts Outdated Show resolved Hide resolved
packages/yarnpkg-core/sources/MessageName.ts Outdated Show resolved Hide resolved
packages/yarnpkg-core/sources/Project.ts Outdated Show resolved Hide resolved
@arcanis
Copy link
Member

arcanis commented Nov 16, 2020

I made a diff refactoring slightly the rendering - in particular:

  • The warnings are now printed in their own section. I saw your comment in feat(core): show warnings on unused / redundant package extensions  #2091 (comment), but I think it's important to put them aside because they are warnings that can usually be actioned right now, whereas the "invalid peer deps" are much trickier to address.

  • I've tweaked the wording to directly reference the field names (rather than > / >> / etc). I feel like it makes the output quite a bit clearer, and decrease the amount of prior knowledge required.

  • Also slightly tweaked the wording itself to try to explain what those warning mean.

  • I've refactored the Report class to make them take responsibility for printing steps or not when they have no actual content. It's important, because this way people can disable the warnings using the new logFilter option and it'll have the right behaviour (previously the steps would still have been printed because install() would have had no idea that the messages would have been hidden).

  • I've prevented packageExtensions from overwriting fields. It's technically breaking, but it wasn't meant to behave this way, and without this it'd be difficult to know which rules are used (for instance in our own repo there are two extraneous ones that didn't get catch because the peer dependencies had different ranges).

I think this looks really good! Your PR will be very useful to many 👍

@paul-soporan paul-soporan changed the title feat(core): show warnings on unused / unneeded package extensions feat(core): show warnings on unused / redundant package extensions Nov 16, 2020
@arcanis arcanis merged commit 871bd75 into master Nov 17, 2020
@arcanis arcanis deleted the paul/feat/core/validate-package-extensions branch November 17, 2020 11:28
thermarthae added a commit to thermarthae/eslint-config that referenced this pull request Nov 30, 2020
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.

2 participants