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 build for React next #726

Closed
kentcdodds opened this issue Jun 29, 2020 · 33 comments
Closed

Fix build for React next #726

kentcdodds opened this issue Jun 29, 2020 · 33 comments
Assignees
Labels

Comments

@kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Jun 29, 2020

Not sure exactly how/when this happened, but the build is failing: https://travis-ci.org/github/testing-library/react-testing-library/builds/703268660

Help welcome!

@MatanBobi
Copy link
Collaborator

@MatanBobi MatanBobi commented Jun 30, 2020

This looks like a problem with the cleanup in the flushing queued microtasks step in some Node environments..
I've tried to reproduce it on my local env with no luck, I'll try to reproduce it today and then see what's the specific problem.

@nickmccurdy
Copy link
Member

@nickmccurdy nickmccurdy commented Jun 30, 2020

Is this an intermittent failure where the first build (before it was rerun with Travis cron) just happened to fail by chance? Or could it be that a dependency updated in a way that changes this behavior?

@marcosvega91
Copy link
Member

@marcosvega91 marcosvega91 commented Jun 30, 2020

I don't think is something regarding to a dependency update. As I see from the log, it happens only on some versions of node.

@MatanBobi
Copy link
Collaborator

@MatanBobi MatanBobi commented Jun 30, 2020

@nickmccurdy
It looks like it started happening quite around the area of pushing the cleanup of queued microstasks and it always fails on the same node version: 12, 14 and current.
It's weird that the initial merge did pass and so did one after that with the same commit as all others who failed..
image

@MatanBobi
Copy link
Collaborator

@MatanBobi MatanBobi commented Jun 30, 2020

Just updating that I tried to reproduce this issue with both of the node versions that failed: 12.18.1, 14.4.0 on my local env but all of the tests passed.

@kentcdodds
Copy link
Member Author

@kentcdodds kentcdodds commented Jun 30, 2020

The fact that #1171 passed (which introduced the microtask change) is interesting 🤔

@eps1lon
Copy link
Member

@eps1lon eps1lon commented Jul 1, 2020

The tests are failing for react@next and react-dom@next. Make sure you reproduce the CI environment by doing npm install react@next react-dom@next which is executed in these builds (https://travis-ci.org/github/testing-library/react-testing-library/jobs/703268662#L234)

@marcosvega91
Copy link
Member

@marcosvega91 marcosvega91 commented Jul 1, 2020

good point, you're right, now the issue is reproducible

@MatanBobi
Copy link
Collaborator

@MatanBobi MatanBobi commented Jul 1, 2020

The tests are failing for react@next and react-dom@next. Make sure you reproduce the CI environment by doing npm install react@next react-dom@next which is executed in these builds (https://travis-ci.org/github/testing-library/react-testing-library/jobs/703268662#L234)

That's a good point, it also explains why it happens only in CRON since only there we use react@next and react-dom@next. As @marcosvega91 stated, the issue reproduced now.
I'll dig into it today.

@kentcdodds
Copy link
Member Author

@kentcdodds kentcdodds commented Jul 1, 2020

Ah yeah, that explains why it's failing for the cron job 😅 Thanks @eps1lon! I forgot we do that.

@nickmccurdy
Copy link
Member

@nickmccurdy nickmccurdy commented Jul 1, 2020

Perhaps we should add react@next to the build matrix for all builds rather than just cron builds so we can tell immediately if a new feature is failing on the current next release. Travis also shows multiple jobs in a build side by side, so for example we'd know if a PR failed on @next but not @latest. FWIW I think this would be more similar to Zeit/Vercel Next's usage of GitHub actions for react@next testing.

@kentcdodds
Copy link
Member Author

@kentcdodds kentcdodds commented Jul 1, 2020

Nah, I don't want to prevent a release because our tests don't work with an unreleased version of react.

@nickmccurdy
Copy link
Member

@nickmccurdy nickmccurdy commented Jul 1, 2020

Good point, we should be able to work around that by adding the @next job as an allowed failure so we can still see the build status without preventing an automated release.

@kentcdodds
Copy link
Member Author

@kentcdodds kentcdodds commented Jul 1, 2020

That sounds fine to me

nickmccurdy added a commit that referenced this issue Jul 1, 2020
- Tests with both React stable and next for all builds (push and cron)
- Allow next releases to fail the build so we can still release
- Solves cron issues like #726 that are only caused by next releases
@nickmccurdy nickmccurdy mentioned this issue Jul 1, 2020
1 of 4 tasks complete
@MatanBobi
Copy link
Collaborator

@MatanBobi MatanBobi commented Jul 2, 2020

Updating what I found until now:
It looks like in react@latest our useEffect return callback is being called before we get to await flush() in our cleanup, that way the setImmediate is being registered as expected before the enqueueTask within flushMicroTasks.
In react@next, that's not the behavior. our useEffect return callback is being called after we get to await flush(), that's why the setImmediate is being registered after the enqueueTask so we're not waiting for it.

This looks like a change in the unmount behavior, I'm trying to find the root cause in react.

@MatanBobi
Copy link
Collaborator

@MatanBobi MatanBobi commented Jul 2, 2020

Ok, I found the change..
It looks like react@next inside commitUnmount calls enqueuePendingPassiveHookEffectUnmount which dispatches the flushPassiveEffects within a scheduleCallback:

    scheduleCallback(NormalPriority, function () {
      flushPassiveEffects();
      return null;
    });

This actually makes the flushPassiveEffects calling our useEffect's destroy function be a different one caused by an internal event in react's event system which triggers commitBeforeMutationEffects, so this isn't a sync operation like it used to.

In react@latest, inside commitUnmount they run a task with priority within the commit phase that just calls the destroy hooks:

var priorityLevel = renderPriorityLevel > NormalPriority ? NormalPriority : renderPriorityLevel;
runWithPriority$1(priorityLevel, function () {
  var effect = firstEffect;
 do {
     var _destroy = effect.destroy;
         if (_destroy !== undefined) {
            safelyCallDestroy(current, _destroy);
          }
          effect = effect.next;
  } while (effect !== firstEffect);
});

runWithPriority is basically sync operation within react@latest.
All of this happens within the ReactDOM.unmountComponentAtNode(container) we do in the cleanup.
In react@next when we call unmountComponentAtNode they schedule a callback to flushPassiveEffects which happens later on, after we setImmediate to flush all microtasks.

Any ideas on how to try and fix this would be great :)

@kentcdodds kentcdodds closed this in 3c9d7b4 Jul 2, 2020
@kentcdodds kentcdodds reopened this Jul 2, 2020
@nickmccurdy nickmccurdy changed the title Fix build Fix build for React next Jul 3, 2020
@MatanBobi
Copy link
Collaborator

@MatanBobi MatanBobi commented Jul 3, 2020

I think we can do something like this for react@next, this will ensure we schedule a callback after react schedule a callback to run the destroy hooks..
The test passes but it seems pretty shady to me, let me know what you think:

import {
  unstable_scheduleCallback as scheduleCallback,
  unstable_NormalPriority as normalPriority,
} from 'scheduler'

function cleanup() {
  mountedContainers.forEach(cleanupAtContainer)
  // flush microtask queue after unmounting in case
  // unmount sequence generates new microtasks
  return {
    then(resolve) {
      scheduleCallback(normalPriority, async () => {
        await flush()
        resolve()
      })
    }
  }
}
@kentcdodds
Copy link
Member Author

@kentcdodds kentcdodds commented Jul 3, 2020

Interesting! Could we put that logic within flush itself?

@MatanBobi
Copy link
Collaborator

@MatanBobi MatanBobi commented Jul 3, 2020

@kentcdodds Yeah, just tried this one and it's also working:

export default function flushMicroTasks() {
  return {
    then(resolve) {
      if (getIsUsingFakeTimers()) {
        // without this, a test using fake timers would never get microtasks
        // actually flushed. I spent several days on this... Really hard to
        // reproduce the problem, so there's no test for it. But it works!
        jest.advanceTimersByTime(0)
        resolve()
      } else {
        scheduleCallback(normalPriority, () => {
          enqueueTask(() => {
            resolve()
          })
        })
      }
    },
  }
}
@kentcdodds
Copy link
Member Author

@kentcdodds kentcdodds commented Jul 3, 2020

Sweet. Will that work for all versions of react we support? If not, can we make it?

@MatanBobi
Copy link
Collaborator

@MatanBobi MatanBobi commented Jul 3, 2020

This solution works with react@latest and react@16.9 but in react@16.8 the scheduleCallback signature was different and the first argument is the callback not the priority.
I guess we're still supporting 16.8 so we might need to think of a way to polyfill it for 16.8 and lower.
Should I work on a PR for this one?

@kentcdodds
Copy link
Member Author

@kentcdodds kentcdodds commented Jul 3, 2020

Yes please. Thanks!

@MatanBobi MatanBobi self-assigned this Jul 4, 2020
@MatanBobi
Copy link
Collaborator

@MatanBobi MatanBobi commented Jul 4, 2020

A few questions I bumped into and would like to see what you all think:
Since I'm not installing the scheduler package and counting on react to install it, I get:
'scheduler' should be listed in the project's dependencies. Run 'npm i -S scheduler' to add it
This might mean we will need to add it as a peer dependency for this fix (I did this in https://github.com/MatanBobi/react-testing-library/tree/bugs/726-React-next-fails-build).
The second problem I encountered was that the scheduler package isn't being added to version 16.5 and below, so this means we will need to polyfill the whole package for these versions.
For version 16.6-16.8 we can just polyfill the function (since the function signature has changed starting from 16.9).
Would love your inputs to see how I can push this forward :)

@kentcdodds
Copy link
Member Author

@kentcdodds kentcdodds commented Jul 4, 2020

We'll probably need something similar to our act-compat.js

I'm thinking we could add it to this code:

try {
// read require off the module object to get around the bundlers.
// we don't want them to detect a require and bundle a Node polyfill.
const requireString = `require${Math.random()}`.slice(0, 7)
const nodeRequire = module && module[requireString]
// assuming we're in node, let's try to get node's
// version of setImmediate, bypassing fake timers if any.
enqueueTask = nodeRequire.call(module, 'timers').setImmediate
} catch (_err) {

+ const globalObj = typeof window === 'undefined' ? global : window

+ let Scheduler = globalObj.Scheduler

  try {
    // read require off the module object to get around the bundlers.
    // we don't want them to detect a require and bundle a Node polyfill.
    const requireString = `require${Math.random()}`.slice(0, 7)
    const nodeRequire = module && module[requireString]
    // assuming we're in node, let's try to get node's
    // version of setImmediate, bypassing fake timers if any.
+   Scheduler = nodeRequire.call(module, 'scheduler')
    enqueueTask = nodeRequire.call(module, 'timers').setImmediate
  } catch (_err) {
    // ...
  }

+ const scheduleCallback = Scheduler ? (Scheduler.scheduleCallback || Scheduler.unstable_scheduleCallback) : (_, cb) => cb() 
+ const NormalPriority = Scheduler ? (Scheduler.NormalPriority || Scheduler.unstable_NormalPriority) : null

I believe that should work.

@kentcdodds
Copy link
Member Author

@kentcdodds kentcdodds commented Jul 4, 2020

Oh, and I guess if there is no scheduler we should fallback to a function that does nothing but call the callback it's given. Should work fine.

@kentcdodds
Copy link
Member Author

@kentcdodds kentcdodds commented Jul 4, 2020

Updated my code above to account for this

@MatanBobi
Copy link
Collaborator

@MatanBobi MatanBobi commented Jul 4, 2020

Didn't know we have act-compat :) Thanks for that @kentcdodds!
Based on the example you showed, I just need to add a specific case for React 16.8 where unstable_scheduleCallback was there but accepts different arguments (callback as the first argument and options as the second).
I'll do something like we do in act-compat and create the PR.
Thanks for the help!

@MatanBobi MatanBobi mentioned this issue Jul 5, 2020
0 of 4 tasks complete
kentcdodds pushed a commit that referenced this issue Jul 5, 2020
* fix: Fix build on react@next

* Working on ^16.9 and lower than 16.8

* Solution works in 16.8 also

* Fixes from code review

* Import only the function we need
@kentcdodds
Copy link
Member Author

@kentcdodds kentcdodds commented Jul 5, 2020

🎉 This issue has been resolved in version 10.4.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kentcdodds kentcdodds added the released label Jul 5, 2020
@nickmccurdy
Copy link
Member

@nickmccurdy nickmccurdy commented Jul 6, 2020

Is this ready to be closed?

@kentcdodds
Copy link
Member Author

@kentcdodds kentcdodds commented Jul 6, 2020

Yep! Weird it wasn't automatically closed 🤔

@kentcdodds kentcdodds closed this Jul 6, 2020
@nickmccurdy
Copy link
Member

@nickmccurdy nickmccurdy commented Jul 6, 2020

I think it's because #726 was only mentioned normally, but you need to prefix it with something like fix/fixes/resolve/resolves to have it be automatically closed.

@MatanBobi
Copy link
Collaborator

@MatanBobi MatanBobi commented Jul 6, 2020

That's probably on me, sorry.
I saw that the PR wasn't linked automatically and didn't understand why. Thanks @nickmccurdy!

@nickmccurdy
Copy link
Member

@nickmccurdy nickmccurdy commented Jul 6, 2020

No worries, just wanted to give the tip

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

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.