Refactor declaration-block-no-duplicate-properties #6545

fpetrakov · 2022-12-23T17:30:43Z

Which issue, if any, is this issue related to?

Is there anything in the PR that needs further explanation?

No, it's self-explanatory.

changeset-bot · 2022-12-23T17:30:46Z

⚠️ No Changeset found

Latest commit: af60e72

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Mouvedia · 2022-12-23T20:37:31Z

lib/rules/declaration-block-no-duplicate-properties/index.js

+				}
+
+				if (!important && duplicateImportant) {
+					decl.remove();


stylistic

I prefer the form return fn();.
ditto for l183, l108, etc

Mouvedia · 2022-12-23T20:46:52Z

lib/rules/declaration-block-no-duplicate-properties/index.js

-									} else {
-										removePreviousDuplicate(decls, lowerProp);
-									}
+						if (!important && duplicateImportant) {


minor

put !important && duplicateImportant in a const
(being reused at l145)

Mouvedia · 2022-12-23T20:55:25Z

lib/rules/declaration-block-no-duplicate-properties/index.js


+				if (ignoreDiffValues || ignorePrefixlessSameValues) {
+					// fails if duplicates are not consecutive
+					if (indexDuplicate !== decls.length - 1) {


suggestion

put indexDuplicate !== decls.length - 1 in a const foo (dunno about the name but you should)
and use !foo at l178

Mouvedia · 2022-12-23T20:59:07Z

perf

verify that it's at least marginally equivalent
https://github.com/stylelint/stylelint/blob/main/docs/developer-guide/rules.md#improve-the-performance-of-a-rule

fpetrakov · 2022-12-24T06:27:59Z

perf

verify that it's at least marginally equivalent https://github.com/stylelint/stylelint/blob/main/docs/developer-guide/rules.md#improve-the-performance-of-a-rule

before:

after:

Mouvedia · 2022-12-24T16:21:17Z

lib/rules/declaration-block-no-duplicate-properties/index.js

+					if (ignorePrefixlessSameValues) {
+						// fails if values of consecutive, unprefixed duplicates are equal
+						if (vendor.unprefixed(value) !== vendor.unprefixed(duplicateValue)) {
+							if (!context.fix) {


It seems like you have

if (!context.fix) { return report({ message: messages.rejected(prop), node: decl, result, ruleName, word: prop, }); } if (duplicateIsMoreImportant) { return decl.remove(); } return duplicateDecl.remove();

twice.

What about

// false if values of consecutive, unprefixed duplicates are equal const foo = vendor.unprefixed(value) !== vendor.unprefixed(duplicateValue; const bar = ignorePrefixlessSameValues && foo; if (!duplicatesAreConsecutive || bar) { … }

at line 107?

Mouvedia · 2022-12-26T13:20:41Z

a separate function

You are correct you can reuse

const foo = () => {
	if (!context.fix) {
		return report({
			message: messages.rejected(prop),
			node: decl,
			result,
			ruleName,
			word: prop,
		});
	}

	if (duplicateIsMoreImportant) {
		return decl.remove();
	}

	return duplicateDecl.remove();
};

at line l113 and l151.

Mouvedia · 2022-12-26T16:03:40Z

lib/rules/declaration-block-no-duplicate-properties/index.js

-						} else {
-							removePreviousDuplicate(decls, lowerProp);
-						}
+					if (!duplicateDecl) return;


either this one is not necessary or the one at l97 is
i.e. if we reach l123 duplicateDecl is true because it's a const

at l97 you have

if (!duplicateDecl) { return; }

the l123 check is called at l133 and l157 which are after l97; since you chose to use a function declaration it's being hoisted hence it could have been called before l97.
It seems to me that duplicateDecl cannot be undefined at that stage because Boolean(undefined) === false hence it would have returned early.
If it's a type check problem you can ask @ybiquitous for help.
e.g. a /** @type … */ comment may be necessary

To remove duplicate if (!duplicateDecl) return;, we need to convert function fixOrReport to const fixOrReport. Because using function causes hoisting.

- function fixOrReport() { + const fixOrReport = () => {

ybiquitous

Thank you. LGTM 👍🏼

jeddy3

@fpetrakov Thank you for the refactor. It reads well.

fpetrakov · 2022-12-28T17:14:59Z

@fpetrakov Thank you for the refactor. It reads well.

Glad to help 🙌

a bit of refactoring here and there mainly to decrease nesting

cd61645

Mouvedia reviewed Dec 23, 2022

View reviewed changes

stylistic fix return fn

a17bf24

fpetrakov added 2 commits December 24, 2022 09:34

put important in const

3740f4b

put duplicate are consecutive in a const

f14506f

fpetrakov requested review from Mouvedia and jeddy3 and removed request for Mouvedia December 24, 2022 16:20

Mouvedia reviewed Dec 24, 2022

View reviewed changes

move unprefixed values check in variable

c0608c1

move fix and report to a separate function

2fd05f6

Mouvedia reviewed Dec 26, 2022

View reviewed changes

fix func decl

af60e72

ybiquitous approved these changes Dec 28, 2022

View reviewed changes

ybiquitous mentioned this pull request Dec 28, 2022

Release 14.16.1 #6540

Closed

6 tasks

jeddy3 approved these changes Dec 28, 2022

View reviewed changes

jeddy3 merged commit 121acce into stylelint:main Dec 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor declaration-block-no-duplicate-properties #6545

Refactor declaration-block-no-duplicate-properties #6545

fpetrakov commented Dec 23, 2022

changeset-bot bot commented Dec 23, 2022 •

edited

Mouvedia Dec 23, 2022 •

edited

Mouvedia Dec 23, 2022

Mouvedia Dec 23, 2022 •

edited

Mouvedia commented Dec 23, 2022

fpetrakov commented Dec 24, 2022

Mouvedia Dec 24, 2022

Mouvedia commented Dec 26, 2022

Mouvedia Dec 26, 2022

Mouvedia Dec 27, 2022 •

edited

ybiquitous Dec 28, 2022

ybiquitous left a comment

jeddy3 left a comment

fpetrakov commented Dec 28, 2022

Refactor declaration-block-no-duplicate-properties #6545

Refactor declaration-block-no-duplicate-properties #6545

Conversation

fpetrakov commented Dec 23, 2022

changeset-bot bot commented Dec 23, 2022 • edited

⚠️ No Changeset found

Mouvedia Dec 23, 2022 • edited

Choose a reason for hiding this comment

Mouvedia Dec 23, 2022

Choose a reason for hiding this comment

Mouvedia Dec 23, 2022 • edited

Choose a reason for hiding this comment

Mouvedia commented Dec 23, 2022

fpetrakov commented Dec 24, 2022

Mouvedia Dec 24, 2022

Choose a reason for hiding this comment

Mouvedia commented Dec 26, 2022

Mouvedia Dec 26, 2022

Choose a reason for hiding this comment

Mouvedia Dec 27, 2022 • edited

Choose a reason for hiding this comment

ybiquitous Dec 28, 2022

Choose a reason for hiding this comment

ybiquitous left a comment

Choose a reason for hiding this comment

jeddy3 left a comment

Choose a reason for hiding this comment

fpetrakov commented Dec 28, 2022

changeset-bot bot commented Dec 23, 2022 •

edited

Mouvedia Dec 23, 2022 •

edited

Mouvedia Dec 23, 2022 •

edited

Mouvedia Dec 27, 2022 •

edited