Use for...of loops to avoid object inheritance#3381
Conversation
alexpetros
left a comment
alexpetros
left a comment
There was a problem hiding this comment.
Generally approve, just left a small comment
| } | ||
| // @ts-ignore tsc doesn't seem to properly handle types merging | ||
| return obj1 | ||
| return Object.assign({}, obj1, obj2) |
There was a problem hiding this comment.
caniuse says this goes back to 2015, basically: https://caniuse.com/mdn-javascript_builtins_object_assign
There was a problem hiding this comment.
Ah, OK, do we have a policy on minimum supported browser version?
There was a problem hiding this comment.
Not an official one, just noting it to say that I think it's okay!
There was a problem hiding this comment.
yeah this is fully supported as this is ES6/ES2015 and we already have many features used from this version when we did v2.0. We should probably decide on a better baseline we want to rule in or out like ES2020 or ES2018 as i've seen ES2016 things like array.includes is already in the code base
| detail = { value: detail } | ||
| } | ||
| triggerEvent(elt, eventName, detail) | ||
| for (const eventName of Object.keys(triggers || {})) { |
There was a problem hiding this comment.
I think const triggers = parseJSON(triggerBody) || {} would be better
alexpetros
added
the
ready for review
|
I think this is good 👍 |
Stop using `for...in` loops because they iterate over the objects' entire inheritance tree. Instead, only iterate over the objects' _own_ keys. This _should_ speed htmx up a bit, but I haven't benchmarked it! It's more of a correctness issue. Also use `Object.assign()` instead of reimplementing it.
3 participants
Description
Stop using
for...inloops because they iterate over the objects' entire inheritance tree. Instead, only iterate over the objects' own keys.This should speed htmx up a bit, but I haven't benchmarked it! It's more of a correctness issue.
Also use
Object.assign()instead of reimplementing it.Testing
Ran
npm run test- all tests passing. This is purely an internal (refactoring) change so no new tests needed.Checklist
masterfor website changes,devforsource changes)
approved via an issue
npm run test) and verified that it succeeded