chore: remove reagent override because it is currently not effective #22320
+14
−2
Conversation
This file contains hidden or 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
Member
Jenkins BuildsClick to see older builds (24)
|
ilmotta
reviewed
Mar 17, 2025
Contributor
ilmotta
left a comment
ilmotta
left a comment
There was a problem hiding this comment.
If this wasn't working for so long, I think it's safe to remove it. On the other hand, should we override reagent.impl.batching/next-tick var to "fix the workaround"? And while doing so, shouldn't we demonstrate the override is better than the default behavior since years have passed and the override may not be necessary anymore?
Contributor
Author
|
@ilmotta Yeah good questions, I had similar questions and discussed them with @flexsurfer I think the conclusion we reached is that overriding the behaviour could have unforeseen consequences because of anything that relies of the subtle timings For example, if were to override So while we could correct the override, I'm not sure how it is needed. It seemed to be related to performance concerns in the past based on this PR: #10403
|
ilmotta
approved these changes
Mar 19, 2025
seanstrom
force-pushed
the
seanstrom/remove-reagent-override
branch
4 times, most recently
from
8ca57b4 to
ef5e7f2
Compare
flexsurfer
reviewed
Mar 25, 2025
|
|
||
| ;;;; re-frame RN setup | ||
| (set! interop/next-tick js/setTimeout) | ||
| (set! batching/fake-raf #(js/setTimeout % 0)) |
Contributor
There was a problem hiding this comment.
i would just comment this line, and add comment why it was commented
seanstrom
force-pushed
the
seanstrom/remove-reagent-override
branch
from
f78b391 to
0c1a037
Compare
seanstrom
commented
Mar 26, 2025
Comment on lines
+38
to
+50
| ;; Note: In the past we've configured reagent to run its batch rendering | ||
| ;; faster by overriding the next-tick function. This technique could be useful | ||
| ;; if we want to adjust the batch speed for different frame-rates since by | ||
| ;; default reagent tunes its batch rendering for 60FPS. | ||
| ;; | ||
| ;; For example, this code would have batches rendering as fast as possible | ||
| ;; (set! reagent.impl.batching/next-tick js/setImmediate) | ||
| ;; | ||
| ;; While this example would approximate 120FPS | ||
| ;; (set! reagent.impl.batching/next-tick #(js/setTimeout % 8)) | ||
| ;; | ||
| ;; And under the hood this is what reagent will use for approximately 60FPS: | ||
| ;; (set! reagent.impl.batching/next-tick #(js/setTimeout % 16)) |
Contributor
Author
There was a problem hiding this comment.
@flexsurfer I've added a comment explaining a little about the override with some example code. It's a little different that what you requested, but I hope it's still okay.
Small note:
In these comments, I reference overriding next-tick instead of fake-raf. Which this would be correct override moving forward (at least for now). So if we do end up wanting to un-comment these lines, they will work as intended.
Let me know what you think 🙏
Member
50% of end-end tests have passedNot executed tests (2)Failed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Expected to fail tests (2)Click to expandClass TestWalletCollectibles:
Passed tests (7)Click to expandClass TestWalletOneDevice:
Class TestWalletCollectibles:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
|
flexsurfer
approved these changes
Mar 27, 2025
pavloburykh
force-pushed
the
seanstrom/remove-reagent-override
branch
from
0c1a037 to
b616d28
Compare
Member
69% of end-end tests have passedFailed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestWalletOneDevice:
Class TestWalletCollectibles:
Passed tests (11)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletCollectibles:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
Contributor
|
@seanstrom thanks for the PR. E2E failures are not PR related. Ready for merge. |
seanstrom
force-pushed
the
seanstrom/remove-reagent-override
branch
from
b616d28 to
0302b4e
Compare
This change removes an override for reagent's polyfill for requestAnimationFrame. Initially, the codebase used an override to configure to use a different polyfill, but now the override is not currently effective because of how the override is set. The `fake-raf` def is used by the `next-tick` def inside reagent, but both of those defs are evaluated before the override is set, meaning that the override does not take effect since we're not re-evaluating the next-tick def.
seanstrom
force-pushed
the
seanstrom/remove-reagent-override
branch
from
0302b4e to
c85a5c1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Summary
fake-rafdef is used by thenext-tickdef inside reagent, but both of those defs are evaluated before the override is set, meaning that the override does not take effect since we're not re-evaluating the next-tick def.Platforms
Steps to test
This change should not affect anything since it removes code that was not working, so it might be simplest to avoid manual testing unless it's just starting the app.
status: ready