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

Ensure "addEventListener" exists on "window" for "scheduler" package #13731

Merged
merged 4 commits into from Sep 26, 2018

Conversation

Projects
None yet
4 participants
@trueadm
Contributor

trueadm commented Sep 26, 2018

This fixes #13694.

When the scheduler package initializes, it tries to attach an event listener to window via addEventListener. In some environments, the window object might be mocked and thus addEventListener might be missing. This PR adds a check to ensure that this function does exist before trying to attach the event.

@trueadm trueadm requested a review from gaearon Sep 26, 2018

@gaearon

I think it would be more consistent to do this in the existing typeof window === 'undefined' check.

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 26, 2018

It would also be nice to get an understanding of what changed that made this a problem for RN.

@trueadm

This comment has been minimized.

Contributor

trueadm commented Sep 26, 2018

@gaearon Why would you do it in the typeof window === 'undefined' condition, shouldn't it be done in the else of that condition (which is where it is)? If window is undefined then we never even touch the logic to add the event listener. My understanding is that it's because this package was pulled in to RN environments where they are using a mocked window that doesn't have addEventListener on it.

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 26, 2018

I mean that for our purposes “window without addEventListener” is just as bad as “undefined window”. So we should treat them the same way — by falling back.

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 26, 2018

My understanding is that it's because this package was pulled in to RN environments

This is what I’d like to clarify. It’s not clear to me how it got pulled in. Do we know where it’s imported?

@sizebot

This comment has been minimized.

sizebot commented Sep 26, 2018

Details of bundled changes.

Comparing: d0c0ec9...c35bf9f

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD
scheduler.development.js +0.9% +1.1% 20.55 KB 20.73 KB 5.55 KB 5.61 KB NODE_DEV
scheduler.production.min.js 🔺+1.0% 🔺+0.5% 4.49 KB 4.54 KB 1.77 KB 1.78 KB NODE_PROD
Scheduler-dev.js +0.9% +1.1% 20.81 KB 21 KB 5.59 KB 5.65 KB FB_WWW_DEV
Scheduler-prod.js 🔺+0.4% 🔺+0.4% 12.68 KB 12.73 KB 2.74 KB 2.75 KB FB_WWW_PROD

Generated by 🚫 dangerJS

@trueadm

This comment has been minimized.

Contributor

trueadm commented Sep 26, 2018

@gaearon I understand what you mean now, thanks for clarifying.

So my primary fix for this was to remove the require to react-dom in RN etc, but there isn't one that I could see anywhere. The most likely case for react-dom being pulled in, which pulls in scheduler, is that people are using third-party React packages that pull it in. Given we can't manage these third-party libraries, I applied the fix as a conditional (I've applied changes as per your feedback) to scheduler itself.

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 26, 2018

The most likely case for react-dom being pulled in, which pulls in scheduler, is that people are using third-party React packages that pull it in.

Ah. This makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment