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

support React v16.0 #42

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

support React v16.0 #42

wants to merge 2 commits into from

Conversation

mattotodd
Copy link

@mattotodd mattotodd commented Sep 26, 2017

Table of Contents

Description

uses 'create-react-class' module instead of React.createClass per 15.5 migration notes

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@robhuzzey
Copy link

+1 to this... wouldn't we want to bump the package up as a major change? I assume this is a breaking change?

@kiwidamien
Copy link

kiwidamien commented Jan 2, 2018

👍, current version has breaking errors when I use it.
I notice the last comment is in November, is anyone currently working on PRs in this repo?

@kiwidamien
Copy link

kiwidamien commented Jan 2, 2018

For anyone else with this issue, the problem is with the debugger using createClass to make its component. A simple work-around if you are not using the debugger is to simply delete the line

module.exports = {
  Experiment: require("./lib/Experiment"),
  Variant: require("./lib/Variant"),
  emitter: require("./lib/emitter"),
  //experimentDebugger: require("./lib/debugger"), <-- remove this line.
  mixpanelHelper: require("./lib/helpers/mixpanel"),
  segmentHelper: require("./lib/helpers/segment")
};

@jacobc-eth
Copy link

Is there any update on this PR?

@okovpashko
Copy link

@jacobcantele I see no activity from maintainers in this repo. My requests #40 and #41 were created about 4 months ago and there's still no activity also.
I start thinking about creating a fork and fixing all issues there along with some new features.

@jacobc-eth
Copy link

I would 100% support that.

@moretti
Copy link

moretti commented Feb 19, 2018

Any update on this?
Maybe pushtell (cc @wehriam @JonShort) could update the README, if they're looking for maintainers.
Would be a shame to fork and publish it as separate npm module just to support React 16.

@JonShort
Copy link

I'd recommend forking the library, I was made a contributor to one of the pushtell repos a while ago after a PR i did, but i've had to fork their projects since because I don't think they're operating anymore.

Luckily their repos are pretty easy to work with, I'm just not sure who has publish rights for the npm package/s.

@moretti
Copy link

moretti commented Feb 21, 2018

I published a new version under @mavelapp/react-ab-test compatible with React 16:
https://github.com/marvelapp/react-ab-test

@okovpashko
Copy link

@moretti did you publish your fork on NPM?

@moretti
Copy link

moretti commented Feb 28, 2018

@okovpashko yeah
https://www.npmjs.com/package/@marvelapp/react-ab-test
I rewrote the existing tests in jest and upgraded the deps. I’ll see if I can fix the existing issues when I have some time.

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 this pull request may close these issues.

None yet

7 participants