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

React 15.5 compatibility #918

Merged
merged 9 commits into from
May 3, 2017
Merged

React 15.5 compatibility #918

merged 9 commits into from
May 3, 2017

Conversation

inukshuk
Copy link
Contributor

@inukshuk inukshuk commented Apr 8, 2017

Thanks for react-intl! This PR just imports PropTypes from 'prop-types' package as suggested by React 15.5 and later.

@ghostd ghostd mentioned this pull request Apr 10, 2017
React 0.14.9 supports the separate prop-types module required
by 0.15.5 and later.
Starting with React 15.5 `createRenderer` is provided by
react-test-renderer.
React 15.5 and later exports `createRenderer` via 'react-test-renderer'
while React 14 uses 'react-addons-test-utils'. Because of the static
nature of ES6 import statements we cannot conditionally import from
different modules depending on React versions; therefore, this commit
adds an NPM pretest script to generate 'test/renderer.js' which simply
imports the right module for the current React version. This is to make
sure tests work with both `npm run react:14` and `npm run react:15`, but
ideally should be dropped once support for React 14 is dropped.
@inukshuk
Copy link
Contributor Author

I also updated the examples to use 'prop-types' and changed the minimum peer dependency of React to version 0.14.9 (which is compatible with the 'prop-types' module).

I was a bit puzzled how to make the tests work with both React 14 and 15.5+ without deprecation messages. ES6 import statements must be static and at the top-level so we cannot conditionally import createRenderer from either 'react-addons-test-utils' (for React 14) or 'react-test-renderer/shallow' (for React 15). For the time being I solved this by creating 'test/renderer.js' in an NPM pretest script to make sure all tests continue to work for the react:14 and react:15 targets but I'd be happy if anyone could suggest a better solution. (Or if the react:14 tests can be dropped)

@inukshuk inukshuk changed the title React 15.5 compatibility: Use PropTypes from 'prop-types' React 15.5 compatibility Apr 13, 2017
@@ -1,4 +1,5 @@
import React, {Component, PropTypes} from 'react';
import React, {Component} from 'react';
import {PropTypes} from 'prop-types';

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! I must be going blind.

@bjankord
Copy link

@inukshuk This was a solution I saw from @kentcdodds to handle importing different packages based on what version of react was used. This idea could be used to conditionally import createRenderer from either 'react-addons-test-utils' (for React 14) or 'react-test-renderer/shallow' (for React 15)

@inukshuk
Copy link
Contributor Author

@bjankord cool thanks!

Are the tests ever compiled to something other than commonjs modules? If that's not a requirement we can just use require directly -- that's much better!

@inukshuk
Copy link
Contributor Author

Alright, followed @bjankord's suggestion and used require directly; I think this should cover all compatibility issues now.

@quantizor
Copy link

@ericf could you take a look at this please? FB shows console errors for these deprecations and it adds gross noise when trying to develop / debug

@eMarek
Copy link

eMarek commented Apr 19, 2017

Any plans when this will be merged or have I missed something that we are waiting for?

@MichaelDeBoey
Copy link

@ericf I think this can be merged?

@tswaters
Copy link

Last commit from @ericf was almost a month ago, updating contributing docs for posting a new release.

Also there's this: https://ericf.me/apple. I'm not sure if ericf is working on this project any longer and/or there are any other contributors.... most of the time I see someone with a contributor badge it's ericf.

@MichaelDeBoey
Copy link

@antoinerousseau is the reviewer, so maybe he can merge it then?
Or maybe @ericf knows who's following up this project in this place?

@quantizor
Copy link

quantizor commented Apr 24, 2017

I wouldn't hold your breath for him to respond, tbh. Yahoo is all but dead at this point, would be nice if the FB open source team might be interested in taking it over? @gaearon

@MichaelDeBoey
Copy link

That could indeed be another possibility @probablyup :-)

@gaearon
Copy link

gaearon commented Apr 24, 2017

We don’t really “take over” projects from other companies—and we also don’t maintain anything that we aren’t actively using in production. We have our own internalization infra at Facebook so it wouldn’t make sense for us to steer react-intl. If there is a need for a new maintainer, perhaps you could raise an issue? I’m sure some of the large companies using it could volunteer to help!

@MichaelDeBoey MichaelDeBoey mentioned this pull request Apr 24, 2017
@okuryu
Copy link
Member

okuryu commented Apr 24, 2017

Just the other day I got a commit bit for react-intl from @ericf and @redonkulus (from Yahoo) to maintain react-intl. I have told them I would like to help this project. I think this change is a high priority for us, but please give me a little time to review this change and prepare to release a new version.

All feedbacks about this change are always welcome, please post your thoughts if you have any concern or idea.

@bjankord
Copy link

Thanks for the update @okuryu!

@MichaelDeBoey
Copy link

MichaelDeBoey commented Apr 27, 2017

Any news on this one @okuryu? 🙂

@lyushenko
Copy link

We are slightly blocked by this issue and I'd like to contribute if you need any help @okuryu

@bjankord
Copy link

Any update on this? @ericf @okuryu This is the last dependency I need updated to have react 15.5 compatibility before I can update to react 15.5.

@mikebridge
Copy link

This is a trivial PR that remains unmerged after three weeks. Is this project still active or is it a victim of Yahoo's collapse?

@bmv437
Copy link

bmv437 commented May 1, 2017

@mikebridge please read this comments above. Specifically this one. I expect @okuryu will merge this PR soon and cut a new release.

@mikebridge
Copy link

@bmv437 Thanks, I have read the comments which is why I'm concerned. I've only just started using react-intl, and I now get the impression that the core team has suddenly left the project—is this not correct?

@okuryu okuryu merged commit 4c7a766 into formatjs:master May 3, 2017
@okuryu
Copy link
Member

okuryu commented May 3, 2017

Okay, I've merged. I'll release a new version within a week.

@gaearon
Copy link

gaearon commented May 3, 2017

Just to be clear, this is really more of a 16 than 15.5 compatibility. Your apps will work even with warnings—it’s nice to gradually fix them as we move towards 16, but I think it’s also unnecessary to exert so much pressure on library maintainers right now. This change is not urgent.

@robcaldecott
Copy link

Understood 100% but as this warning takes the form of a console error it causes tests to fail. Does anyone know of a good workaround for this?

@lyushenko
Copy link

@robcaldecott you can use the production version of React to suppress console errors or install a fork temporarily:

"react-intl": "inukshuk/react-intl"

However, it would be better if React logged warnings instead of errors (console.warn)

@quantizor
Copy link

quantizor commented May 3, 2017 via email

@robcaldecott
Copy link

I use create-react-app and that does not come with Jest configured to use the production version of React. I'll try setting NODE_ENV and see if that works.

@gaearon
Copy link

gaearon commented May 3, 2017

I don't think console.error causes tests to fail in Create React App. What gave you that impression?

However, it would be better if React logged warnings instead of errors

It's planned for 15.6.

@gaearon
Copy link

gaearon commented May 3, 2017

(If it does cause CRA tests to fail please raise an issue with CRA. This was never intended. In general when you see weird behavior please raise issues rather than silently accept it 😉 )

@robcaldecott
Copy link

On Windows with CI=true it returns an ERRORLEVEL of 1. I'll raise a PR. Weirdly it does not do this if you pass -- --coverage to npm test.

@lyushenko
Copy link

@gaearon in our case, we're using Capybara to run feature tests and it is set up to capture all JS errors and our tests are failing due to deprecation warning. No big deal though, we kinda "hacked" it :)

@gaearon
Copy link

gaearon commented May 3, 2017

Interesting. Please do raise it, thanks!

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