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 14 and 15 testing identified warning in react 15 #183

Merged
merged 3 commits into from
Apr 21, 2016

Conversation

export-mike
Copy link
Contributor

I have identified warnings regarding the new warning added in react 15,

as discussed on react blog and github PR
https://facebook.github.io/react/blog/2016/04/07/react-v15.html#new-deprecations-introduced-with-a-warning

facebook/react#5048

This PR contains how to identify the problem:

new run script:
npm run test:react:15

[16:01:26] Starting 'test'...

  Warning: Input elements must be either controlled or uncontrolled (specify either the value prop, or the defaultValue prop, but not both). Decide between using a controlled or uncontrolled input element and remove one of these props. More info: https://fb.me/react-controlled-components

  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․Warning: `value` prop on `input` should not be null. Consider using the empty string to clear the component or `undefined` for uncontrolled components.
․․․․․․
  ․Warning: Textarea elements must be either controlled or uncontrolled (specify either the value prop, or the defaultValue prop, but not both). Decide between using a controlled or uncontrolled textarea and remove one of these props. More info: https://fb.me/react-controlled-components
․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․

  76 passing (549ms)

[16:01:29] Finished 'test' after 2.52 s

npm run test:react:14

[16:02:07] Using gulpfile ~/projects/bybox/react-typeahead/gulpfile.js
[16:02:07] Starting 'test'...


  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․

  76 passing (377ms)

[16:02:08] Finished 'test' after 1.74 s

I will submit a following PR to show how this will be fixed.

@export-mike
Copy link
Contributor Author

we need to move away from using defaultValue as its now a react thing and not just a react-typeahead thing. defaultValue is for uncontrolled components we can use value. http://facebook.github.io/react/docs/forms.html#controlled-components

@export-mike
Copy link
Contributor Author

I've bumped to 2.0.0 to move from using defaultValue to using initial value, tests passing.

@export-mike
Copy link
Contributor Author

testing scripts inspired by airbnb's enzyme testing library. https://github.com/airbnb/enzyme/blob/master/package.json

@export-mike
Copy link
Contributor Author

anyone waiting on this theres a public fork on @pebblecode/react-typeahead

@fmoo
Copy link
Owner

fmoo commented Apr 19, 2016

@export-mike - any preference on going straight to 2.0.0 vs doing a 2.0.0-rc.1?

@fmoo
Copy link
Owner

fmoo commented Apr 19, 2016

Any guidance on whether this conflicts with #167? If I'm going to version bump to 2.0.0 I'd like to make sure we get other breaking changes in at the same time.

cc @nosilleg

@nosilleg
Copy link
Contributor

nosilleg commented Apr 19, 2016

@fmoo This dropped off my radar. I had made some v2 changes and was in the process of setting up React Storybook to try and get some use cases documented. I've done a hasty patch to try and get rid of that aspect, and there's a big TODO left for inputProps, but this is the general direction that I was going...

https://github.com/fmoo/react-typeahead/compare/master...nosilleg:v2?expand=1

Another change would have been to remove defaultValue since its not valid for a controlled component.

@export-mike
Copy link
Contributor Author

@fmoo no preference, happy to version the release candidate(s), @nosilleg funny you should mention storybook I thought about this yesterday. 👍 so is there much left todo?

@fmoo
Copy link
Owner

fmoo commented Apr 21, 2016

I suck at npm, but this seems fishy: with the react 15 peerDep bump, should we bump react-dom and react-addons-test-utils as well?

npm WARN [email protected] requires a peer of react@^0.14.8 but none was installed.
npm WARN [email protected] requires a peer of react@^0.14.8 but none was installed.

@fmoo fmoo merged commit 40d858e into fmoo:master Apr 21, 2016
@fmoo
Copy link
Owner

fmoo commented Apr 21, 2016

Released as [email protected]

@export-mike
Copy link
Contributor Author

We shouldnt need to bump peer dependencies, within this branch I've added npm i [email protected] and npm i react@15 as part of test scripts, this way we can support both. @fmoo what version of npm are you on?

@export-mike
Copy link
Contributor Author

Just a thought let's update the npm test script to call npm run test:all?

@export-mike
Copy link
Contributor Author

Cool I can test this out in a few hours I'm using my phone atm

@fmoo
Copy link
Owner

fmoo commented Apr 21, 2016

what version of npm are you on?

npm --version
3.6.0

I'm on linux (Ubuntu)

I installed react 15.0.1 globally

@export-mike
Copy link
Contributor Author

export-mike commented Apr 21, 2016

If you remove react from global dependencies and use the new
npm run test:react:14/15

On Thu, 21 Apr 2016 06:04 Peter Ruibal, [email protected] wrote:

what version of npm are you on?

npm --version
3.6.0

I'm on linux (Ubuntu)

I installed react 15.0.1 globally


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#183 (comment)

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.

3 participants