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

Remove useless spread and fallbacks #249

Merged
merged 4 commits into from
Mar 5, 2025

Conversation

fisker
Copy link
Contributor

@fisker fisker commented Mar 4, 2025

Was reading code, and found some cases that unicorn can prevent.

If you don't want keep the rules, I can remove them.

BTW: The eslint config seems not working right, when I run eslint --fix, there are more change (some are wrong) applied, maybe because of version of dependencies.

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@ota-meshi
Copy link
Member

The CI error seems to be from the main branch, I will fix that later.

@ota-meshi ota-meshi merged commit b925331 into vuejs:master Mar 5, 2025
0 of 9 checks passed
@fisker fisker deleted the no-useless-fallback-in-spread branch March 5, 2025 02:42
@@ -95,7 +95,7 @@ export function extractGeneric(element: VElement): GenericProcessInfo | null {
typeDefScope: Scope,
isRemoveTarget: (nodeOrToken: HasLocation) => boolean,
) {
for (const variable of [...typeDefScope.variables]) {
for (const variable of typeDefScope.variables) {
Copy link
Member

Choose a reason for hiding this comment

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

It's just sharing, we had to copy the original array because it might be modified, so we'll undo this modification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, sorry about it.

Copy link
Member

Choose a reason for hiding this comment

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

The tests should have caught that, but it seems I broke them. Sorry 😅

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