Skip to content

Conversation

@timmywil
Copy link
Member

@timmywil timmywil commented Mar 22, 2024

Summary

Differences are:

  • use diffChars when both expected and actual are strings
  • use diffWords for everything after that

Objects, arrays, and numbers are diffed as before.

Checklist

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

One comment

@timmywil timmywil force-pushed the diff-words branch 2 times, most recently from f093c59 to 23ccedc Compare March 23, 2024 17:19
Comment on lines 81 to 85
// Diff everything else as words
diff = Diff.diffWords(
quoteWrap( error.expected ),
quoteWrap( error.actual )
);
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 the necessary insight here is that we want to diff type-mismatched values after serialization, which quoteWrap a) fails to convey, and b) currently does so inconsistently (e.g., it escapes " in strings by \-prefixing, but does not similarly escape \).

Suggested change
// Diff everything else as words
diff = Diff.diffWords(
quoteWrap( error.expected ),
quoteWrap( error.actual )
);
// Diff everything else as words (post-serialization)
diff = Diff.diffWords(
serializeForDiff( error.expected ),
serializeForDiff( error.actual )
);

with

function serializeForDiff( value ) {
	// Use naive serialization for everything except types with confusable values
	if ( typeof value === "string" ) {
		return JSON.stringify(value);
	} else if ( typeof value === "bigint" ) {
		return `${ value }n`;
	} else if ( typeof value === "symbol" ) {
		const ctor = Symbol.keyFor(value) !== undefined ? "Symbol.for" : "Symbol";
		return `${ctor}(${JSON.stringify(value.description)})`;
	}
	return `${ value }`;
}

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 like that solution.

Copy link
Member

Choose a reason for hiding this comment

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

I like that, too! Using JSON.stringify on strings is a great idea. ❤️

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'll try it and post some examples.

Copy link
Member Author

@timmywil timmywil Mar 26, 2024

Choose a reason for hiding this comment

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

Something I realized while testing is that Symbols do not get sent over the wire (they get removed when passed through JSON.stringify). So, I moved that part to listeners.js.

image
image

Copy link
Member Author

@timmywil timmywil Mar 26, 2024

Choose a reason for hiding this comment

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

Also, I switched to just show the diff all the time when at least one is non-null. I found it reassuring to confirm those are the values in diff form.

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

Thanks for following with the review! LGTM, Richard’s suggestions also look good to apply before merging.

timmywil and others added 2 commits March 26, 2024 16:12
@timmywil timmywil merged commit b9d333a into jquery:main Mar 27, 2024
@timmywil timmywil deleted the diff-words branch March 27, 2024 14:36
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Development

Successfully merging this pull request may close these issues.

3 participants