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

fix(flushMicroTasks): Fallback to no scheduler in case of exception #739

Merged

Conversation

MatanBobi
Copy link
Member

Closes #736

What: When we couldn't require the scheduler package we will now fallback to isModernScheduleCallbackSupported = false meaning we will use callback => callback().

Why: Following the conversation in #736 - We found this to be a proper solution for now in case the Scheduler wasn't loaded.
I still think we need to discuss this issue further and think if we can be able to figure a different way to require the Scheduler at runtime.

How: I've added an if statement in the catch block to see if isModernScheduleCallbackSupported is true, in that case I change it to false and write an error to console.

I think that we may need to separate the try catch to two different ones and handle each error for it's own. Update me what you think please :)
Also, within the error message we may want to link to the documentation stating how to fix this one.

Checklist:

  • Documentation added to the
    docs site - We need to implement a documentation change for this one, please update me where you think it fits.
  • Tests - N/A
  • Typescript definitions updated - N/A
  • Ready to be merged

@MatanBobi
Copy link
Member Author

@kentcdodds - I created this one following the conversation in #736, I still think as @eps1lon pointed out that we should talk about a proper solution since this sounds more like a band-aid than a way to fix the problem :/

src/flush-microtasks.js Outdated Show resolved Hide resolved
src/flush-microtasks.js Outdated Show resolved Hide resolved
@kentcdodds
Copy link
Member

As for whether we should not use the scheduler at all, I'd love to avoid it as well, but I know for a fact that without these changes, we run into some serious issues when using fake timers. I spent two days banging my head against the wall and this is the solution.

I'd argue that act is an implementation detail as well, but we as the testing library, handle that for our users so they don't have to worry about it themselves. We do the same thing for cleanup. So yes, I agree that using the scheduler is an implementation detail of React, but it's our job to shield our users from implementation details. If we could avoid it that would be great, but considering we can't... 🤷‍♂️

@kentcdodds kentcdodds merged commit 9e5cf59 into testing-library:master Jul 8, 2020
@MatanBobi
Copy link
Member Author

MatanBobi commented Jul 8, 2020

So yes, I agree that using the scheduler is an implementation detail of React, but it's our job to shield our users from implementation details. If we could avoid it that would be great, but considering we can't... 🤷‍♂️

@kentcdodds I agree with the approach and I sure think that this change is required.
My only thought is that it looks like we're experiencing problems with bundling systems and us not being able to require the module..

@kentcdodds
Copy link
Member

Yeah, luckily most of the time it shouldn't matter. It only seems to matter in some edge cases. And in those cases, people using bundlers can simply set window.Scheduler = require('scheduler') and everything will work fine. I'm ok with that trade-off.

@MatanBobi
Copy link
Member Author

MatanBobi commented Jul 8, 2020

Yeah, luckily most of the time it shouldn't matter. It only seems to matter in some edge cases. And in those cases, people using bundlers can simply set window.Scheduler = require('scheduler') and everything will work fine. I'm ok with that trade-off.

Cool. Do you think we should add it in the docs/readme?
That was my initial thought when I added the console.error - to reference the ones experiencing this issue to a fix.

@kentcdodds
Copy link
Member

The problem is for older versions of React which don't use/need the scheduler. I suppose we could add a little bit of logic to target folks who are using a version of React that uses the scheduler but we can't access it. If you want to do that, please do!

@kentcdodds
Copy link
Member

🎉 This PR is included in version 10.4.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scheduleFn is calling NormalPriority as a function rather than cb
2 participants