search

fmoo / react-typeahead

Pure react-based typeahead and typeahead-tokenizer
ISC License
677 stars 232 forks source link

react 14 and 15 testing identified warning in react 15 #183

Closed export-mike closed 8 years ago

export-mike commented 8 years ago

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

https://github.com/facebook/react/pull/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 commented 8 years ago

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 commented 8 years ago

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

export-mike commented 8 years ago

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

export-mike commented 8 years ago

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

fmoo commented 8 years ago

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

fmoo commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

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 react-addons-test-utils@0.14.8 requires a peer of react@^0.14.8 but none was installed.
npm WARN react-dom@0.14.8 requires a peer of react@^0.14.8 but none was installed.
fmoo commented 8 years ago

Released as react-typeahead@2.0.0-alpha.1

export-mike commented 8 years ago

We shouldnt need to bump peer dependencies, within this branch I've added npm i react@0.14 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 commented 8 years ago

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

export-mike commented 8 years ago

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

fmoo commented 8 years ago

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 commented 8 years ago

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, notifications@github.com 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 https://github.com/fmoo/react-typeahead/pull/183#issuecomment-212746852