-
Notifications
You must be signed in to change notification settings - Fork 104
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
Return empty arrays for performance.getEntries, other relevant methods #240
Conversation
|
||
this.clock.uninstall(); | ||
|
||
delete Performance.prototype.getEntries; |
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 am just wondering about the cleanup bit. Will the original functions now be "restored"?
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.
Just added a test that the original functions are restored, it does appear to work.
I also have a test for Cypress that runs in actual Chrome that shows the restoration working IRL: https://github.com/cypress-io/cypress/pull/4108/files#diff-c0a131a0b58412db68e166e9d7a16383R75
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 think he asked about the restoration within tests since you override and delete
from prototype
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.
Hmm, I don't see what you mean. Is there something else that needs to be tested?
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.
dunno, maybe something like
const origGetEntries = Performance.prototype.getEntries;
const origGetEntriesByName = Performance.prototype.getEntriesByName;
const origGetEntriesByType = Performance.prototype.getEntriesByType; =
then restore them instead of the delete
? not sure. If the Performance
object is already a fake, then it doesn't matter and I probably misunderstood 🙂
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 could be wrong, but it looks like lolex
just uses the prototype to know what methods to override on the global performance
, so I don't think anything additional needs to be tested here.
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.
@fatso83 is this ready?
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.
Sure. If you and Ben thinks this is fine, go ahead. I didn't mean to be a blocker (too busy these days with 👶)
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 think this is fine :]
Co-Authored-By: flotwig <github@chary.us>
I can do a release next week when I'm back from DevDays |
@benjamingr Ping, would be awesome to see this released so we can incorporate this fix downstream :) |
doing it |
Purpose (TL;DR)
Fix issues caused by applications which expect
performance.getEntries
,performance.getEntriesByName
, andperformance.getEntriesByType
to return arrays (such as Facebook SDK - downstream issue: cypress-io/cypress#3625)Right now, lolex clobbers these with
() => undefined
. Just using() => []
instead would fix this.Background (Problem in detail)
MDN page for Performance functions: https://developer.mozilla.org/en-US/docs/Web/API/Performance
Only 5 have non-undefined return types. The other 2 (besides
getEntries.*
) are:now()
- already done by lolextoJSON()
- not handled, not sure if there's a good way to mock this valueSolution
Just have those 3 methods return
[]
whenlolex.install()
is called, instead ofundefined