Cleaning up code by making vnode.attrs always non-null

[kfule]

Commit f9e5163 made vnode.attrs always non-null, so there is no need for code to make vnode.attrs null or assume vnode.attrs is null.

Description

The following two processes have been removed

• Workaround for vnode of textarea when vnode.attrs is null
• Processing of key removal to set vnode.attrs to null

For the latter, the behavior will change when there is only key in attrs (key is not removed from attrs). Some tests have also been changed to ensure that the changes are appropriate.

Motivation and Context

There is still a not very useful code that assumes that vnode.attrs will be null. Removing that code will reduce the amount of code and improve the outlook of the process.

How Has This Been Tested?

Running "npm run test"

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation change

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have updated docs/changelog.md

[dead-claudia]

If it's truly redundant, the tests shouldn't need modified. Please revise this such that tests can pass as-is.

I suspect your intuition is right, but the patch as written is not.

[pygy]

@dead-claudia we're ignoring "key" in setAttr(), and, after this patch, in order to not have it there would mean either

1. deleting it from the user supplied object, or
2. iterating over the attrs to make sure it is the only one and replace attrs with an empty object.

When there are other props key is left on the attrs.

I'd rather avoid 1).

Regarding 2) I assume that setAttr() is inlined in setAttrs() and updateAttrs(), and "key" is the first check we make in there. So looking for a lone key in execSelector will not save anything perf-wise.

[dead-claudia]

@pygy My concern wasn't perf, but in unnecessary breakage and ensuring it's only removing the truly redundant parts.

Any changes to the tests reduces my confidence in the removed parts being truly redundant, if it helps.

[pygy]

Understood but in this case I'm not sure the tests are right. If the intent was to filter key out of the attrs of DOM vnodes, the current logic is broken, and only partially tested. We don't test for m("x", {key, a}), and we keep the key in that scenario.

[kfule]

Thanks for the review.

I understand your motivation for not wanting to change the test, but please consider that the changed test is changed by the commit f9e5163 (which made attrs always non-null), too.
f9e5163#diff-048b8c57f4a13b5784f875e7a5c31e2c2a6502c38e64c31d619e0212e5586e74R274
f9e5163#diff-048b8c57f4a13b5784f875e7a5c31e2c2a6502c38e64c31d619e0212e5586e74R346

iterating over the attrs to make sure it is the only one and replace attrs with an empty object.

As a result of removing just such a process, the test needed to be changed...
https://github.com/MithrilJS/mithril.js/pull/2819/files#diff-60b8d5249940bc09c5a4d14138469f40b3def8e635b25fbd59ff61493ec10581L67

[kfule]

To make the purpose of this pr easier to understand, I have re-modified the code with minimal changes.

I have reverted back to the original logic for the following lines, but there seems to be a bug when sharing attrs, so I would like to issue a separate issue or pr.
https://github.com/MithrilJS/mithril.js/pull/2819/files#diff-60b8d5249940bc09c5a4d14138469f40b3def8e635b25fbd59ff61493ec10581L41

[JAForbes]

Looks good to me, let's at least get it into next and we can see if there's any observable perf difference in the wild?

[dead-claudia]

Closed due to the next branch being deleted. Thanks to https://github.com/orgs/community/discussions/139697, I can't resurrect this PR, so it'd have to be re-created against main.

@JAForbes If you get a chance, can you figure out how this fares in npm run bench with and without this patch? If it's not significant, you can file a PR yourself with this PR reapplied and I'll merge it.

[JAForbes]

Will try to find the time, if not @kfule I think we'd be happy to merge this if you could re-open the PR against main and run npm run bench before/after the change and verify there's no substantial change.

[kfule]

@JAForbes ok, I will re-open the PR against main.

[kfule]

I re-opened the PR at #2977.