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: React next fails on build (#726) #732

Merged

Conversation

MatanBobi
Copy link
Member

@MatanBobi MatanBobi commented Jul 5, 2020

What: I've added a scheduler-compat module which will mock unstable_scheduleCallback for version of react that don't have the Scheduler package.

Why: Following #726, our build was failing for react@next so we needed to change the way we flush micro tasks.

How: I took act-compat as an example and took @kentcdodds code example here.
The only thing I added was an error handler because I had to suppress the error.

Checklist:

  • Documentation added to the docs site - Let me know if any documentation for this one is needed.

  • Tests - I'm working on tests for this, it's failing in CI for Coverage.

  • Typescript definitions updated - N/A

  • Ready to be merged

This is a Draft PR just to get initial feedback, the code isn't good enough yet.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 5, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5d9f64d:

Sandbox Source
new-wildflower-zlfoo Configuration
dank-cache-7m81x Configuration

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! What do you think of my suggestions?

src/flush-microtasks.js Show resolved Hide resolved
Comment on lines 65 to 69
scheduleCallback(null, () => {
enqueueTask(() => {
resolve()
})
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the change above, we no longer need this first argument

Suggested change
scheduleCallback(null, () => {
enqueueTask(() => {
resolve()
})
})
scheduleCallback(() => enqueueTask(resolve))

Comment on lines 1 to 76
const globalObj = typeof window === 'undefined' ? global : window

let Scheduler = globalObj.Scheduler
try {
const requireString = `require${Math.random()}`.slice(0, 7)
const nodeRequire = module && module[requireString]
// import React's scheduler so we'll be able to schedule our tasks later on.
Scheduler = nodeRequire.call(module, 'scheduler')
} catch (_err) {
console.error("The react version you're using doesn't support Scheduling")
}
// in case this react version has a Scheduler implementation, we use it,
// if not, we just create a function calling our callback
const NormalPriority = Scheduler
? Scheduler.NormalPriority || Scheduler.unstable_NormalPriority
: null

const isScheduleCallbackSupported = Scheduler !== undefined
let isModernScheduleCallbackSupported = null

const errorHandler = e => {
// If the error originated from Scheduler, it means we're in v16.8.6
if (
e.message === 'callback is not a function' &&
e.filename.includes('scheduler')
) {
console.error(e.error.stack, e.error.detail)
e.preventDefault()
} else {
console.error(e.error)
}
}

export default function scheduleCallback(_, cb) {
if (!isScheduleCallbackSupported) {
return cb()
}

if (isModernScheduleCallbackSupported === null) {
// patch console.error here
const originalConsoleError = console.error
console.error = function error(...args) {
/* if console.error fired *with that specific message* */
/* istanbul ignore next */
const firstArgIsString = typeof args[0] === 'string'
if (
firstArgIsString &&
args[0].indexOf('TypeError: callback is not a function') === 0
) {
// v16.8.6
isModernScheduleCallbackSupported = false
globalObj.removeEventListener('error', errorHandler)
console.error = originalConsoleError
return cb()
} else {
originalConsoleError.apply(console, args)
console.error = originalConsoleError
return cb()
}
}

globalObj.addEventListener('error', errorHandler)
return Scheduler.unstable_scheduleCallback(NormalPriority, () => {
console.error = originalConsoleError
isModernScheduleCallbackSupported = true
globalObj.removeEventListener('error', errorHandler)
return cb()
})
} else if (isModernScheduleCallbackSupported === false) {
return cb()
}

return Scheduler.unstable_scheduleCallback(NormalPriority, cb)
}

/* eslint no-console:0 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this file anymore.

Suggested change
const globalObj = typeof window === 'undefined' ? global : window
let Scheduler = globalObj.Scheduler
try {
const requireString = `require${Math.random()}`.slice(0, 7)
const nodeRequire = module && module[requireString]
// import React's scheduler so we'll be able to schedule our tasks later on.
Scheduler = nodeRequire.call(module, 'scheduler')
} catch (_err) {
console.error("The react version you're using doesn't support Scheduling")
}
// in case this react version has a Scheduler implementation, we use it,
// if not, we just create a function calling our callback
const NormalPriority = Scheduler
? Scheduler.NormalPriority || Scheduler.unstable_NormalPriority
: null
const isScheduleCallbackSupported = Scheduler !== undefined
let isModernScheduleCallbackSupported = null
const errorHandler = e => {
// If the error originated from Scheduler, it means we're in v16.8.6
if (
e.message === 'callback is not a function' &&
e.filename.includes('scheduler')
) {
console.error(e.error.stack, e.error.detail)
e.preventDefault()
} else {
console.error(e.error)
}
}
export default function scheduleCallback(_, cb) {
if (!isScheduleCallbackSupported) {
return cb()
}
if (isModernScheduleCallbackSupported === null) {
// patch console.error here
const originalConsoleError = console.error
console.error = function error(...args) {
/* if console.error fired *with that specific message* */
/* istanbul ignore next */
const firstArgIsString = typeof args[0] === 'string'
if (
firstArgIsString &&
args[0].indexOf('TypeError: callback is not a function') === 0
) {
// v16.8.6
isModernScheduleCallbackSupported = false
globalObj.removeEventListener('error', errorHandler)
console.error = originalConsoleError
return cb()
} else {
originalConsoleError.apply(console, args)
console.error = originalConsoleError
return cb()
}
}
globalObj.addEventListener('error', errorHandler)
return Scheduler.unstable_scheduleCallback(NormalPriority, () => {
console.error = originalConsoleError
isModernScheduleCallbackSupported = true
globalObj.removeEventListener('error', errorHandler)
return cb()
})
} else if (isModernScheduleCallbackSupported === false) {
return cb()
}
return Scheduler.unstable_scheduleCallback(NormalPriority, cb)
}
/* eslint no-console:0 */

@@ -1,3 +1,5 @@
import scheduleCallback from './scheduler-compat'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think having the compat file is necessary.

Suggested change
import scheduleCallback from './scheduler-compat'
import React from 'react'
import semver from 'semver'
const globalObj = typeof window === 'undefined' ? global : window
let Scheduler = globalObj.Scheduler
const isModernScheduleCallbackSupported = semver.satisfies(React.version, '>=16.8.6')

Also, let's move this code to below the comments, just above the getIsUsingFakeTimers

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're definitely right about this one, I thought we don't want to use semver.. This was my go-to solution but since act-compat isn't using it and I didn't see it in the project, I didn't want to add it.
This simplifies everything.

@MatanBobi
Copy link
Member Author

MatanBobi commented Jul 5, 2020

Thanks for this! What do you think of my suggestions?

I'm on it, sorry for making this PR too exhaustive, didn't think we'll be good with using semver :)
Thanks for the comments!

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kentcdodds kentcdodds marked this pull request as ready for review July 5, 2020 17:14
@kentcdodds kentcdodds merged commit 604d3e9 into testing-library:master Jul 5, 2020
@kentcdodds
Copy link
Member

@all-contributors please add @MatanBobi for code

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @MatanBobi! 🎉

@kentcdodds
Copy link
Member

Thanks so much for your help! I've added you as a collaborator on the project (with write access now). Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

@MatanBobi
Copy link
Member Author

Thanks a lot @kentcdodds!

@kentcdodds
Copy link
Member

🎉 This PR is included in version 10.4.4 🎉

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.

2 participants