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

Create Adapter for React Native & React 16 #1436

Open
mcdankyl opened this issue Dec 15, 2017 · 44 comments
Open

Create Adapter for React Native & React 16 #1436

mcdankyl opened this issue Dec 15, 2017 · 44 comments
Labels
feature request Issues asking for stuff that would be semver-minor help wanted

Comments

@mcdankyl
Copy link

mcdankyl commented Dec 15, 2017

Problem Statement

Currently we have no support for using a react native adapter with Enzyme. This is because the current adapters are built for react native web.

Attempting to use mount with the current adapter will cause various issues from polluting the user's console with a ton of logs to inability to import necessary libraries.

While this is the case you can get some testing done by filtering the logs and using jsdom.

Most of the console error issues seem to be coming from the ReactDOM call for the React 16 adapter here

Purpose

The purpose of this ticket is to track work related to implementation and decisions regarding implementation of a react native enzyme adapter.

TODO

  1. Find an alternative to rendering with ReactDOM
  2. Decide best way to use the solution for (1) with existing adapters
  3. Decide whether we would like to do all necessary react native stubbing in the adapter a-la react-native-mock-render, do a RYO implementation, or not take a firm stance to the native mocking
  4. Add unit tests to verify functionality
  5. Update Necessary Docs

Related Issues

#1390
#831

@ljharb
Copy link
Member

ljharb commented Dec 16, 2017

We have one for React 16; but we definitely do need one for react-native.

Implementing it here in this repo would be great; anyone interested in opening a PR?

@ljharb ljharb added feature request Issues asking for stuff that would be semver-minor help wanted labels Dec 16, 2017
@ljharb
Copy link
Member

ljharb commented Dec 16, 2017

Since we’d want a different adapter as needed for different React-native versions, we might also want a utils package. We’d also want to add a new binary to the adapter helper for react-native.

@mcdankyl
Copy link
Author

Yup I'm already aware of the regular react 16 adapter. I was more saying one that works with react native apps built with react 16 as the react dependency. I've been ripping apart and experimenting with the 16 adapter as a way to boost familiarity with the adapter system.

I'm planning to start working on implementation after holiday travels.

Do you have some ideas as to what the adapter would use instead of ReactDOM? I saw that react test utils also has a render method in addition to a shallow render method. I was thinking of investigating that first.

@ljharb
Copy link
Member

ljharb commented Dec 16, 2017

I'm really not sure, unfortunately. I would assume/hope that the react-native team would provide something for this use case.

@mcdankyl
Copy link
Author

Looking to start investigating implementation. I started setting up my development environment and was reading through the existing test suite. I noticed that quite a bit of the tests seem to be heavily geared towards React and not React Native. This isn't really a surprise, but leads me ask how we would prefer to setup the suite for React Native.

A couple quick options I was able to come up with:

  1. Nest directories in enzyme-test-suite/test so that we have enzyme-test-suite/test/react and enzyme-test-suite/test/react-native. Change npm test:only to something like npm test:only:react and react native variant.
  2. Try to refactor the existing suite to also work with React Native.

Of these (2) sounds to be more maintainable with less duplication. I'm unsure if there are any pitfalls which would block it from being possible due to lack of familiarity with the test suite. Is one of these decisions largely preferable over the other? I'd also be interested to know if there are any other better options.

@ljharb
Copy link
Member

ljharb commented Dec 27, 2017

I’d prefer we move to a place where tests that are specific to each adapter (like anything React-specific) moves to that individual adapter, and the tests in enzyme-test-suite become adapter-agnostic (and thus, react and react-native agnostic)

@mcdankyl
Copy link
Author

mcdankyl commented Dec 27, 2017

That makes the most sense. Would you say that the test suite refactor should be a hard precondition before starting the implementation of the wrapper? Or would it make more sense to just implement the adapter with the right testing setup, allowing the other adapters to be refactored as time permits?

I'm leaning toward the refactor needing done first so that we have thorough code coverage on the native adapter from day one.

Edit: Also, would that fall under #1132 or should I create another issue?

@ljharb
Copy link
Member

ljharb commented Dec 27, 2017

I would hope it could be done first :-) and yes, that’s indeed the issue for it.

@lededje
Copy link

lededje commented Jan 7, 2018

Is there a work around for this, or can someone point me in the right direction to look into this issue? Is the adapter for React Web the best place to start?

@ljharb
Copy link
Member

ljharb commented Jan 7, 2018

I’d start there, yes. Not sure if there’s any existing work.

@lededje
Copy link

lededje commented Jan 8, 2018

I've spent a few hours looking over the adapter react 16 uses and have come to the conclusion it's unlikely something that can be done independently. I'll share that which I've learnt below for other adventurers. The following appears to be the case at time of posting.

There is a single set of tests for all adapters found there. This file runs once for each of the adapter versions in this setup file. Excluding specific version of react from running a set of tests are done in this file with an itIf with a condition provided. If you want to test a react native adapter none of the tests cases in this file will apply to you so you will need to set up an independent test set.

When you switch between each version of react using npm run react:x where x is a version number it removes and installs dependencies as described in this env.js file. When I created another adapter and removed react-dom as a peer dependency all the over version's tests broke.

There is no documentation on how an adapter is meant to work.

Overall it feels that react native was very much left out of the changes made when converting Enzyme to use adapters. I'm not judging; it's Airbnb's library and they can do what they want with it, but it's a shame because you can't adequately test a react native application without Enzyme imo.

I have a workaround solution I'm using that I'll post in the next few minutes.

@lededje
Copy link

lededje commented Jan 8, 2018

This is the work around I'm using for the time being. It's a jest mock set that knocks out the apis that react-native use allowing you to use the standard react 16 adapter.

My implementation is similar to react native's implementation

This is meant as a functioning stop gap until a proper adapter has been built.

@ljharb
Copy link
Member

ljharb commented Jan 8, 2018

All the scripts can and should be modified as needed.

Fwiw tho, this wasn’t a result of any migration - enzyme has never explicitly tested itself on react native nor was it originally built to handle it. Adding more explicit react-native support is going to be an effort that necessitates changing things as needed.

In particular, the organization of the tests is such that (as you discovered) it’s not useful as a generic adapter test suite. The same is true if a preact or inferno adapter tried to use them. This is something we’ll need long term regardless.

@lededje
Copy link

lededje commented Jan 8, 2018

this wasn’t a result of any migration - enzyme has never explicitly tested itself on react native

That's really interesting, after looking deeper I think I've found the duct tape module that allowed react native work.

Would you be interested in doing progressive changes rather than a big bang?

First split out the tests to allow generic adapter tests, then sort out the environment switching mechanisms that require react-dom to be installed etc?

@ljharb
Copy link
Member

ljharb commented Jan 8, 2018

That sounds like a great approach.

The purpose of the "react" scripts is to make sure that for each version of react, the appropriate additional packages (like react-dom) are installed. If we need a "react-native" script, great, if we come up with a better solution, great :-)

@felipecodes
Copy link

Someone working on it?

@mcdankyl
Copy link
Author

@thefelpes No one is working on it from my end. We ended up using React Test Renderer and Shallow render directly.

@acomito
Copy link

acomito commented May 6, 2018

Hi @mcdankyl, what do you mean by "use shallow render directly"?

@mcdankyl
Copy link
Author

mcdankyl commented May 7, 2018

At least for the react 16 adapter, shallow rendering relies on ShallowTestRenderer.
See https://github.com/airbnb/enzyme/blob/master/packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js#L211

We are directly using ShallowTestRenderer for shallow rendering and using TestRenderer for full renders.

@ljharb
Copy link
Member

ljharb commented Jun 26, 2018

It'd still be great to create a RN adapter; using those tools directly takes a lot of work to migrate across major react versions.

@mathewmorris
Copy link

@shawnaxsom Hey man, I appreciate it so much! I'm going to try this out today and see what it can do! 👍🏼 You're awesome!

@salamancajr
Copy link

I was having trouble with this set up that you can find on https://airbnb.io/enzyme/docs/guides/react-native.html
until I looked at the package.json of @shawnaxsom and noticed react-dom still needs to be on your list of dependencies.

@yuanfux
Copy link

yuanfux commented Mar 12, 2019

The following function

function copyProps(src, target) {
    Object.defineProperties(target, {
         ...Object.getOwnPropertyDescriptors(src),
         ...Object.getOwnPropertyDescriptors(target)
     });
}

induces the Error in Node 8

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.

@shawnaxsom Any idea of how to fix this ?

acrabb added a commit to acrabb/hilton that referenced this issue Mar 24, 2019


Looks like it can be done...but I'll end the rabbit hole here.
@htbkoo
Copy link

htbkoo commented Mar 31, 2019

Same here - I am encountering the same issue mentioned by @yuanfux

When I tried to run the setup file, the test failed with the following message:

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
 1: node_module_register
 2: v8::V8::ToLocalEmpty
 3: node::performance::MarkPerformanceMilestone
 4: node::performance::MarkPerformanceMilestone
 5: v8::internal::wasm::SignatureMap::Find
 6: v8::internal::Builtins::CallableFor
 7: v8::internal::Builtins::CallableFor
 8: v8::internal::Builtins::CallableFor
 9: 000002CDCDD043C1

FYI, the versions of dependencies used (I am using enzyme and react with typescript):

"devDependencies": {
    ... 
    "enzyme": "^3.9.0",
    "enzyme-adapter-react-16": "^1.11.2",
    "jest": "^24.5.0",
    "jsdom": "^14.0.0",
    "react": "^16.0.0",
    "react-dom": "^16.0.0",
    "react-test-renderer": "^16.0.0",
    "ts-jest": "^24.0.0",
    "typescript": "^3.3.4000"
    ...
  },

@zanemcca
Copy link

zanemcca commented Jun 4, 2019

@yuanfux @htbkoo using assign from lodash instead of copyProps worked for me.

@gregkerzhner
Copy link

@yuanfux - here is our complete test-setup.js file that seems to compile and work fine


import { JSDOM } from "jsdom";

const jsdom = new JSDOM("<!doctype html><html><body></body></html>");
const { window } = jsdom;

const copyProps = (src, target) => {
  const props = Object.getOwnPropertyNames(src)
    .filter(prop => typeof target[prop] === "undefined")
    .map((prop) => {
    	console.log(JSON.stringify(prop))
    	return Object.getOwnPropertyDescriptor(src, prop)
    });

  Object.defineProperties(target, props);
};

global.window = window;
global.document = window.document;
global.navigator = {
  userAgent: "node.js"
};
copyProps(window, global);

import { configure } from "enzyme";
import Adapter from "enzyme-adapter-react-16";

// Setup enzyme's react adapter
configure({ adapter: new Adapter() });

// Ignore React Web errors when using React Native
console.error = message => {
  return message;
};

@kas-elvirov
Copy link

@yuanfux - here is our complete test-setup.js file that seems to compile and work fine


import { JSDOM } from "jsdom";

const jsdom = new JSDOM("<!doctype html><html><body></body></html>");
const { window } = jsdom;

const copyProps = (src, target) => {
  const props = Object.getOwnPropertyNames(src)
    .filter(prop => typeof target[prop] === "undefined")
    .map((prop) => {
    	console.log(JSON.stringify(prop))
    	return Object.getOwnPropertyDescriptor(src, prop)
    });

  Object.defineProperties(target, props);
};

global.window = window;
global.document = window.document;
global.navigator = {
  userAgent: "node.js"
};
copyProps(window, global);

import { configure } from "enzyme";
import Adapter from "enzyme-adapter-react-16";

// Setup enzyme's react adapter
configure({ adapter: new Adapter() });

// Ignore React Web errors when using React Native
console.error = message => {
  return message;
};

doesnt work for me. The same error. But thanks

@ghasemikasra39
Copy link

Not working for me.

Current behavior

image

Your environment

System:
      OS: Linux 4.15 Ubuntu 18.04.3 LTS (Bionic Beaver)
      CPU: (8) x64 Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz
      Memory: 2.83 GB / 11.63 GB
      Shell: 5.4.2 - /bin/zsh
    Binaries:
      Node: 10.16.3 - ~/.nvm/versions/node/v10.16.3/bin/node
      Yarn: 1.19.0 - /usr/bin/yarn
      npm: 6.9.0 - ~/.nvm/versions/node/v10.16.3/bin/npm
    npmPackages:
      react: 16.8.3 => 16.8.3 
      react-native: 0.59.8 => 0.59.8 

API

  • mount

Version

library version
enzyme 3.10.0
react 16.8.3
react-dom 16.10.1
react-test-renderer 16.8.3
adapter (below) 1.14.0

Adapter

  • enzyme-adapter-react-16

@ljharb
Copy link
Member

ljharb commented Oct 5, 2019

Nothing is going to work properly until a full react native enzyme adapter is created; using jsdom is a workaround.

@mauriciopf

This comment has been minimized.

@jonzakhip

This comment has been minimized.

@Peretz30

This comment has been minimized.

@Mangor1no

This comment has been minimized.

@sirajalam049

This comment has been minimized.

@yogendrajs

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues asking for stuff that would be semver-minor help wanted
Projects
None yet
Development

No branches or pull requests