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

Out of memory when snapshotting react-sortable-hoc #249

Closed
pcvandamcb opened this issue Jul 19, 2017 · 7 comments · Fixed by #260
Closed

Out of memory when snapshotting react-sortable-hoc #249

pcvandamcb opened this issue Jul 19, 2017 · 7 comments · Fixed by #260

Comments

@pcvandamcb
Copy link

Jest/Node crashes with a memory leak when trying to snapshot a component that contains an element connected with react-sortable-hoc.

The Jest developers stated that this is not an issue with the snapshotting, but with the library.

It makes it very hard to write proper tests for anything that involves a sortable.

Simple test case:

import React from 'react';
import { shallow } from 'enzyme';
import serializer from 'enzyme-to-json/serializer';
import { SortableContainer } from 'react-sortable-hoc';

expect.addSnapshotSerializer(serializer);

const Component = () => (
  <div />
);

const Sortable = SortableContainer(Component);

test('jest crashes on snapshot creation', () => {
  const component = shallow(
    <div>
      <Sortable />
    </div>
  );

  expect(component).toMatchSnapshot();
});

Jest 20.0.4, Node 8.1.4 on macOS 10.12.5

The actual error (which Google tells me is basically out of memory): RangeError: Invalid string length

@cameronmcefee
Copy link
Contributor

cameronmcefee commented Jul 25, 2017

After an afternoon of digging into the same issue, I got lucky and got a partial snapshot before it crashed. It turns out, SortableContainer requires contentWindow which ends up serializing essentially most of the environment into the snapshot, which came out at around ~250mb. @clauderic: I wonder, is it possible to handle the window in such a way that it doesn't need to be passed as a prop so it would be left out of the snapshot?

I'm going to spend this evening to see if it's possible to hack up the snapshot serializer to ignore that property in the meantime.

@clauderic
Copy link
Owner

clauderic commented Jul 25, 2017

Hey @cameronmcefee,

Thanks for looking into this! From what I gather from previous discussions on this issue, it was also an issue with Jest snapshots not playing well withfindDOMNode.

The reason contentWindow is a prop is to deal with scenarios where react-sortable-hoc is used within an iframe, though I suppose if that really is the root of the issue then we could just not set a defaultProp value and do something along the lines of

const contentWindow = this.props.contentWindow || window;

@cameronmcefee
Copy link
Contributor

@clauderic I trust your guidance on this, as I'm just pretending to be a competent developer. Without knowing the considerations of the project that seems reasonable though. If I pass the component a prop of contentWindow={null} it fixes the snapshot issue (though obviously breaks all functionality that relies on it)

@cameronmcefee
Copy link
Contributor

If it's of any use from a proof-I'm-not-making-it-up standpoint, here's the first few lines of the snapshot:

exports[`jest crashes on snapshot creation 1`] = `
<div>
  <sortableList(Component)
    axis="y"
    contentWindow={
      Window {
        "ArrayBuffer": [Function],
        "Buffer": [Function],
        "FileReader": [Function],
        "Float32Array": [Function],
        "Float64Array": [Function],
        "Headers": [Function],
        "Int16Array": [Function],
        "Int32Array": [Function],
        "Int8Array": [Function],
        "Request": [Function],
        "Response": [Function],
        "Uint16Array": [Function],
        "Uint32Array": [Function],
        "Uint8Array": [Function],
        "Uint8ClampedArray": [Function],
        "XMLHttpRequest": [Function],
        "__stopAllTimers": [Function],
        "__timers": Object {},
        "_core": Object {
          "Attr": [Function],
          "Blob": [Function],
          "CDATASection": [Function],
          "CSSImportRule": [Function],
          "CSSMediaRule": [Function],
          "CSSRule": [Function],
          "CSSStyleDeclaration": [Function],
          "CSSStyleRule": [Function],
          "CSSStyleSheet": [Function],
          "CharacterData": [Function],
          "Comment": [Function],
          "CustomEvent": [Function],
          "DOMException": [Function],
          "DOMImplementation": [Function],
          "DOMParser": [Function],
          "DOMTokenList": [Function],
          "Document": [Function],
          "DocumentFragment": [Function],
          "DocumentType": [Function],
          "Element": [Function],
          "ErrorEvent": [Function],
          "Event": [Function],
          "EventTarget": [Function],
          "File": [Function],
          "FileList": [Function],
          "FocusEvent": [Function],
          "FormData": [Function],
          "HTMLAnchorElement": [Function],
          "HTMLAppletElement": [Function],

You get the idea.

@clauderic
Copy link
Owner

Yeah that makes a lot of sense. I'll run test case and get back to you with my findings 😄

@cameronmcefee
Copy link
Contributor

👋 I hate to be the "any update on this?" guy, but this issue is preventing me from writing some acceptance tests I'd love to have.

Would it be helpful if I submit a PR for your suggested change above?

@clauderic
Copy link
Owner

Hey @cameronmcefee,

Sorry about the delay, completely forgot to look into this. A PR would be most welcome, especially if you're able to confirm it solves the issue ❤️

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

Successfully merging a pull request may close this issue.

3 participants