-
Notifications
You must be signed in to change notification settings - Fork 783
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
Core: QUnit.equiv inner refactors [Step 1] #1700
Conversation
Small removals/adjustments to make the code more readable.
d2915d7
to
5e5ce7d
Compare
src/equiv.js
Outdated
@@ -103,13 +101,12 @@ const callbacks = { | |||
}, | |||
|
|||
array (a, b) { | |||
const len = a.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this was meant to account for cases where length is a (custom) getter rather than a plain property or (native) array accessor. Having said that, for custom objects we shouldn't be using this function so probably fine? Just leaving here as FYI, this is not a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think this shouldnt be a problem due to a typeOf here: https://github.com/qunitjs/qunit/blob/main/src/equiv.js#L282
src/equiv.js
Outdated
@@ -298,7 +298,7 @@ function innerEquiv (a, b) { | |||
} | |||
|
|||
// ...across all consecutive argument pairs | |||
return arguments.length === 2 || innerEquiv.apply(this, [].slice.call(arguments, 1)); | |||
return arguments.length === 2 || innerEquiv.apply(this, Array.prototype.slice.call(arguments, 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is in a hot path, we may want to cache it in core/utilities like toString
and hasOwn
, which would also avoid the lookup through various lexical scopes to global host object, and then three property chain lookups. This is not a blocker, improvement either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea implementing it now!
Can you elaborate what you mean by this? I understand ES Module cache as being about files executed via I do note as reminder that these are (currently) inlined and compiled to ES5, which may or may not affect the outcome. Perhaps you're measuring this at the distribution layer, e.g. importing the qunit.js artefact and noticing an improvement there by letting it compile these upfront instead of lazily. To my knowledge, JS engines nowadays defer compilation of declared functions until they first run, but that they have special awareness of the IIFE paradigm and eagerly compile those during the top-level file compilation e.g. not re-entering the compiler when the top-level executes the If I understand correctly, this is intended to improve startup time for the script, not runtime for diff operations, correct?
Ack. If the values are strings only, you can use a StringSet shim that uses native Set when available. I added a StringMap for this as well in src/globals.js. I have a 3-line StringSet implementation in MediaWiki that you're welcome to add here. |
Node.js stores/caches module exports as an object to avoid extra filesystem calls. In node.js commonjs imports, this is under Here is how for example I circumvent the module cache by using a querystring on an imported file that has its contents changed, only way I could make node.js not use the cache of the There is probably no performance benefit compared to IIFE as the cost of calling additional function here is negligible but I think its still better to have the code/functions more flat, and always return the module object directly instead of doing the computation/mutation on an import.
👍 I'll add this to |
757fdac
to
4eef297
Compare
4eef297
to
7dfa1cb
Compare
Makes code more readable and removes variable mutations in compareConstructors().
@@ -112,3 +112,25 @@ export const StringMap = typeof g.Map === 'function' && | |||
}); | |||
} | |||
}; | |||
|
|||
export const StringSet = g.Set || function (input) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When landing, I'll change this one to a typeof
ternary to match g.Map
. I tend to not bother adding a type checks for most polyfills (e.g. Array or String prototype methods) as I agree that presence is sufficient. If it's set to something else, that's asking for trouble!
But, global constructors in particular are prone to conflicting with unrelated values. E.g. <h2 id="Set">Set</h2>
in an HTML fixture will result in g.Set
coming through the window
object via WindowProxy as HTMLElement reference. It's one of those legacy features on the web you are likely to run into sooner or later even if you're not (intentionally) looking for it.
https://html.spec.whatwg.org/#named-access-on-the-window-object
Small refactors (removals/adjustments) to make code more readable. Makes code more readable and remove variable mutations. * Remove IIFE closure, moving main function as direct export, and the rest as top-level local function. * callbacks.object: Optimize compareConstructors(). * callbacks.object: Optimize object comparison prop call by skipping overhead and indirection of typeEquiv() for known array type. * callbacks.array: Optimize isContainer() away, using a predefined Set. * innerEquiv: Remove array allocation by re-using `slice` reference. Closes #1700.
I kicked off a browserstack-full run in CI, and noticed that IE11 failed due to an infinite loop. I narrowed this down to the fact that native |
…rted Avoid IE11 failure, where we otherwise fail to do proper recursion checks against cyclical JSON. Ref #1700 (comment)
As part of #1700, there was an inintended change to how we compare constructors. Instead of comparing `obj.constructor` of the actual value, we started comparing the constructor property as it exists on the prototype before the constructor function runs, and thus also before any after-the-fact mutation to an object property. Undo that part of the change and add a regression test. Fixes #1706.
This makes QUnit.equiv code more readable/maintainable and probably faster due to 2 changes:
We can also utilize JS
Set
and remove/inlineisContainer
function altogether here to do faster checks, I left this out from this PR because linter gives a warning telling mecompat/compat: Set is not supported in iOS Safari 7.0-7.1, IE 9
:https://github.com/qunitjs/qunit/blob/main/src/equiv.js#L63
Also JIT compiler might be able to do further optimizations based on other small changes but unlikely.
I suggest reviewing per commit ;)