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

SchedulerDOM falls back to globalThis if window is undefined #20865

Closed

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Feb 23, 2021

In response to user feedback from @rickhanlonii, I recently made a small change to our scheduling profiler so that it stores lane labels in addition to numeric values (#20808).

While re-building the profiler UI to add the newly available info, I found that it throws when importing profiling data because of references to window (e.g. window.setTimeout) inside of SchedulerDOM. (Some of the scheduling profiler code runs in a web worker and window isn't defined there.)

This PR adds a fallback to globalThis if window is not defined. It prefers window if available to minimize change. We could perhaps also workaround this by falling back to something else e.g. self but globalThis seemed better.

Thoughts?

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 23, 2021
@sizebot
Copy link

sizebot commented Feb 23, 2021

Comparing: c62986c...0182034

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 122.36 kB 122.36 kB = 39.41 kB 39.41 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 128.94 kB 128.94 kB = 41.47 kB 41.47 kB
facebook-www/ReactDOM-prod.classic.js = 404.67 kB 404.67 kB = 75.15 kB 75.15 kB
facebook-www/ReactDOM-prod.modern.js = 393.02 kB 393.02 kB = 73.24 kB 73.24 kB
facebook-www/ReactDOMForked-prod.classic.js = 404.68 kB 404.68 kB = 75.15 kB 75.15 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/scheduler/cjs/scheduler.development.js +1.68% 16.45 kB 16.72 kB +2.44% 4.63 kB 4.74 kB
oss-stable/scheduler/cjs/scheduler.development.js +1.68% 16.45 kB 16.72 kB +2.44% 4.63 kB 4.74 kB
facebook-react-native/scheduler/cjs/Scheduler-dev.js +1.67% 16.49 kB 16.77 kB +2.53% 4.62 kB 4.74 kB
facebook-www/Scheduler-dev.classic.js +1.19% 23.76 kB 24.04 kB +1.80% 6.35 kB 6.46 kB
facebook-www/Scheduler-dev.modern.js +1.19% 23.76 kB 24.04 kB +1.80% 6.35 kB 6.46 kB
facebook-react-native/scheduler/cjs/Scheduler-prod.js +0.93% 10.14 kB 10.23 kB +1.11% 2.61 kB 2.64 kB
facebook-react-native/scheduler/cjs/Scheduler-profiling.js +0.93% 10.14 kB 10.23 kB +1.11% 2.61 kB 2.64 kB
facebook-www/Scheduler-prod.classic.js +0.88% 11.41 kB 11.51 kB +0.94% 2.87 kB 2.89 kB
facebook-www/Scheduler-prod.modern.js +0.88% 11.41 kB 11.51 kB +0.94% 2.87 kB 2.89 kB
facebook-www/Scheduler-profiling.classic.js +0.66% 15.11 kB 15.21 kB +0.79% 3.56 kB 3.59 kB
facebook-www/Scheduler-profiling.modern.js +0.66% 15.11 kB 15.21 kB +0.79% 3.56 kB 3.59 kB
oss-experimental/scheduler/cjs/scheduler.production.min.js +0.65% 4.33 kB 4.36 kB +0.94% 1.82 kB 1.83 kB
oss-stable/scheduler/cjs/scheduler.production.min.js +0.65% 4.33 kB 4.36 kB +0.94% 1.82 kB 1.83 kB
oss-stable/react/umd/react.production.min.js +0.35% 10.99 kB 11.03 kB +0.43% 4.43 kB 4.45 kB
oss-experimental/react/umd/react.production.min.js +0.32% 12.04 kB 12.08 kB +0.55% 4.72 kB 4.74 kB
oss-stable/react/umd/react.profiling.min.js +0.30% 13.21 kB 13.25 kB +0.44% 4.98 kB 5.00 kB
oss-experimental/react/umd/react.profiling.min.js +0.27% 14.26 kB 14.30 kB +0.32% 5.27 kB 5.29 kB
oss-stable/react/umd/react.development.js +0.27% 103.79 kB 104.07 kB +0.41% 25.70 kB 25.81 kB
oss-experimental/react/umd/react.development.js +0.27% 106.05 kB 106.33 kB +0.40% 26.12 kB 26.22 kB

Generated by 🚫 dangerJS against 0182034

@gaearon
Copy link
Collaborator

gaearon commented Feb 23, 2021

Is this fixing a regression, or adding a feature? In other words, did it not throw before?

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 23, 2021

I haven't re-built and run the scheduling profiler in a while. My guess is that this broke with #20025 and we just hadn't noticed.

@rickhanlonii
Copy link
Member

I think before we were probably using the equivalent of SchedulerNoDOM, and this change now uses SchedulerDOM: https://github.com/facebook/react/pull/20025/files#diff-1b2464331bec5a14f0c1db975b4ea10aeaa2c4dccdec3c135a1d96faf0a1e57fL34

Probably the right call, but that's why the changes broke. We're using the DOM scheduler in an non-DOM environment. If we land this, we should probably rename this scheduler.

@bvaughn bvaughn closed this Feb 23, 2021
@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 23, 2021

I don't care about this specific fix. I was thinking of SchedulerDOM as SchedulerBrowser so this seemed to make sure. If the "DOM" bit is significant, then okay. I can close this.

I'll dig in more to find out how the scheduling profiler ended up in this code path.

@bvaughn bvaughn deleted the scheduler-dom-global-this-fallback branch February 23, 2021 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants