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(dlx): don't report UNUSED_PACKAGE_EXTENSION warnings #4583

Merged
merged 3 commits into from
Jul 12, 2022

Conversation

paul-soporan
Copy link
Member

What's the problem this PR addresses?

yarn dlx reports UNUSED_PACKAGE_EXTENSION warnings when an extension is unused in the temporary project created by dlx, even if it is used in the actual project the command is run in.

How did you fix it?

Made dlx filter out all UNUSED_PACKAGE_EXTENSION warnings via logFilters since they should only be about the main project.

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.

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

There is a config created on the following line when there is no .yarnrc.yml in the Project which should also be updated.

await xfs.writeFilePromise(targetYarnrc, `enableGlobalCache: ${enableGlobalCache}\nenableTelemetry: false\n`);

(You can also change that to a xfs.writeJsonPromise)

@paul-soporan paul-soporan requested a review from arcanis as a code owner June 28, 2022 23:19
@paul-soporan
Copy link
Member Author

Thanks, completely forgot about that one. I've also taken this opportunity to create a better merge function in miscUtils and use it everywhere since lodash's default one doesn't merge arrays.

@paul-soporan paul-soporan requested a review from merceyz June 28, 2022 23:26
@@ -207,7 +206,7 @@ export default class InitCommand extends BaseCommand {
},
};

merge(editorConfigProperties, configuration.get(`initEditorConfig`));
miscUtils.mergeIntoTarget(editorConfigProperties, configuration.get(`initEditorConfig`));
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need these utils? Does the yaml stringifier supports serializing the comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

lodash's merge doesn't merge arrays, it just overwrites them, so when merging the dlxConfiguration with the project configuration it doesn't merge the logFilters of the two configurations.

Because of this, I needed to implement a util that does merge arrays using lodash's mergeWith, but I realized that we already have a function that does the same thing - the one in sdkUtils that merges our new settings with the .vscode files and preserves CJSON comments.

Because I didn't want to duplicate code, I extracted that one to miscUtils and added more tests and replaced all existing usages of lodash merge / mergeWith for consistency.

Does the yaml stringifier supports serializing the comments?

Nope.

@merceyz merceyz dismissed their stale review July 12, 2022 12:01

Resolved

@arcanis arcanis merged commit 55c3bac into master Jul 12, 2022
@arcanis arcanis deleted the paul/fix/dlx-unused-package-extension-warnings branch July 12, 2022 14:06
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.

3 participants