Skip to content
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

Replace noop's fake Scheduler implementation with mock Scheduler build #14969

Merged
merged 2 commits into from
Feb 28, 2019

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Feb 27, 2019

The noop renderer has its own mock implementation of the Scheduler interface, with the ability to partially render work in tests. Now that this functionality has been lifted into a proper mock Scheduler build, we can use that instead.

Most of the existing noop tests were unaffected, but I did have to make some changes. The biggest one involved passive effects: previously, they were scheduled on a separate queue from the queue that handles rendering. After this change, both rendering and effects are scheduled in the Scheduler queue. I think this is a better approach because tests no longer have to worry about the difference; if you call flushAll, all the work is flushed, both rendering and effects. But for those few tests that do care to flush the rendering without the effects, that's still possible using the yieldValue API.

Follow-up: Do the same for test renderer.

The noop renderer has its own mock implementation of the Scheduler
interface, with the ability to partially render work in tests. Now that
this functionality has been lifted into a proper mock Scheduler build,
we can use that instead.

Most of the existing noop tests were unaffected, but I did have to make
some changes. The biggest one involved passive effects: previously, they
were scheduled on a separate queue from the queue that handles
rendering. After this change, both rendering and effects are scheduled
in the Scheduler queue. I think this is a better approach because tests
no longer have to worry about the difference; if you call `flushAll`,
all the work is flushed, both rendering and effects. But for those few
tests that do care to flush the rendering without the effects, that's
still possible using the `yieldValue` API.

Follow-up: Do the same for test renderer.
@sizebot
Copy link

sizebot commented Feb 27, 2019

Details of bundled changes.

Comparing: 3ada82b...932034c

react-noop-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-noop-renderer.development.js +50.1% +44.1% 27.59 KB 41.41 KB 6.59 KB 9.5 KB NODE_DEV
react-noop-renderer.production.min.js 🔺+25.9% 🔺+27.1% 9.85 KB 12.4 KB 3.3 KB 4.19 KB NODE_PROD
react-noop-renderer-persistent.development.js +49.9% +44.0% 27.7 KB 41.53 KB 6.6 KB 9.51 KB NODE_DEV
react-noop-renderer-persistent.production.min.js 🔺+25.8% 🔺+27.1% 9.88 KB 12.42 KB 3.3 KB 4.2 KB NODE_PROD
react-noop-renderer-server.development.js 0.0% +0.1% 1.83 KB 1.83 KB 876 B 877 B NODE_DEV

Generated by 🚫 dangerJS

@acdlite acdlite merged commit 53e787b into facebook:master Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants