fix(core): Ensure fill only patches functions
#15632
Merged
+117
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lms24
commented
Mar 11, 2025
| @@ -61,7 +61,7 @@ export function supportsDOMException(): boolean { | |||
| * @returns Answer to the given question. | |||
| */ | |||
| export function supportsHistory(): boolean { | |||
| return 'history' in WINDOW; | |||
| return 'history' in WINDOW && !!WINDOW.history; | |||
There was a problem hiding this comment.
Improved this check so that if history was undefined or null we'd also return false here
size-limit report 📦
|
3 tasks
|
I will rework this to more generally fix the wird behaviour in our |
Lms24
commented
Mar 11, 2025
|
|
||
| expect(WINDOW.history).toEqual({ | ||
| replaceState: expect.any(Function), // patched function | ||
| pushState: undefined, // unpatched |
There was a problem hiding this comment.
prior to this PR, pushState would have become a Function, resulting in the wrapper function calling undefined.apply on the "original function".
Lms24
changed the title
window.history propertiesfill only patches functions
andreiborza
approved these changes
Mar 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a guard to our
fillutility that we use to instrument/wrap methods defined on an object. However, due to a misleading type cast infillwe didn't check if the name of the method to be patched actually corresponded to a function on an object.This was surfaced via #15552 where we'd call
fillonwindow.history(pushState|replaceState)without checking if these two methods were actually available or functions.Note: I initially solved this on the
instrumentHistorylevel but then noticed that this is a more general bug infill. Therefore I added some history-specific tests as well but I'd rather keep them than removing them. Fun fact: The more general fix saves ~8 Bytes of bundle size compared to my initial history-based fix 😅closes #15552