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

Search Component: Unknown props error #1141

Closed
imns opened this issue Jan 10, 2017 · 41 comments
Closed

Search Component: Unknown props error #1141

imns opened this issue Jan 10, 2017 · 41 comments

Comments

@imns
Copy link

imns commented Jan 10, 2017

I have a search component with a custom renderer and I keep getting the error Warning: Unknown props _id, url, ... on <div> tag. Remove these props from the element..

The array I pass to results has these extra props, mentioned in the error, but I am using a custom renderer, so it seems to me they should be ignored.

Here is my custom renderer

const resultRenderer = ({ _id, name, url, logo}) => {
    return (
        <div key={_id}>
            <Label content={name}  />
        </div>
    )
}

resultRenderer.propTypes = {
  name: PropTypes.string
}

My component is really simple

export default class CompanyForm extends Component {
    constructor(props) {
        super(props);

        this.state = {
            isLoading: false,
            value: '',
            results: [],
            company: null
        };
    }

    handleResultSelect(e, result) {
        this.setState({
            value: result.name,
            company: result
        });
    }

    handleSearchChange(e, value) {
        this.setState({
            value: value
        });       
        client
            .service('/api/companies')
            .find({
                query: {
                    name: {'$search': value }
                }
            })
            .then(results => {
                this.setState({
                    isLoading: false,
                    results: results.data
                });
            })
            .catch(err => {
                console.error(err);
            })
        
    }

    render() {
        const {isLoading, value, results} = this.state;
                
        return (
            <Search
                loading={isLoading}
                onResultSelect={this.handleResultSelect.bind(this)}
                onSearchChange={ this.handleSearchChange.bind(this)}
                results={results}
                value={value}
                resultRenderer={resultRenderer}
            />
        );
    }
}

The above code is what's causing the warning.

@levithomason
Copy link
Member

levithomason commented Jan 16, 2017

The extra props passed to each result are spread onto the result element's props. See here. This allows you to pass props to the result element as an object. You need to remove these keys from your results before passing your results.data to the results prop.

@levithomason
Copy link
Member

To be clear, a SearchResult component is created for every results object. Each object is used as props. All our components "consume" their own props and pass the extra props along (best practice).

The SearchResult is consuming all the props it knows about and passing the extra props (_id, url) to the div component. Hence the warning.

@koernchen02
Copy link

koernchen02 commented Feb 1, 2017

I have to ask this again...
You are saying the OP has to remove the extra keys from each result object before passing ... results.data to the results prop. But how is it then possible to use the properties in the customRenderer for a single result? In the example code he wants to assign the id as the result-div's key via <div key={_id}> how is this possible if the _id property has gotten removed from each single result?
Is it at all possible, to use custom fields (other than "title", "description", "price" and "image")?

@imns
Copy link
Author

imns commented Feb 1, 2017

All our components "consume" their own props and pass the extra props along (best practice).

This may be the best practice, but annoying in this instance as I assumed defining a custom renderer would allow me to decide what props are passed to the rendered element.

I'd also like to say that I agree with @koernchen02 and either would like to be able to define custom fields like he talks about or have the control over which props get used in the custom renderer.

@levithomason
Copy link
Member

levithomason commented Feb 3, 2017

This may be the best practice, but annoying in this instance as I assumed defining a custom renderer would allow me to decide what props are passed to the rendered element.

Whoa, looks like I misunderstood this one completely guys. Appreciate the extra effort to reach me. The user should definitely have full control over every prop used in all our components.

@jcarbo your eyes appreciated on this:

Looking at the code again, it looks like there is no way for the user to control the props of the <SearchResult /> as the resultRenderer does not render the SearchResult itself but only the children of the SearchResult. Whilst, we are spreading all the original SearchResult props on the actual SearchResult with no hook for the user.

My thinking is that the renderResult should require the user to return a Search.Result or its shorthand. In other words, we should be using a shorthand factory here, SearchResult.create(). This way, they are in full control of the props on the SearchResult itself, and not the children only. Otherwise, they have no way to filter the props that are applied to the SearchResult.

Thoughts?

@imns
Copy link
Author

imns commented Feb 3, 2017

Returning a SearchResult seems like a decent solution. As you said it would give anyone full control over what props are passed to it.

@iamdanthedev
Copy link

iamdanthedev commented Feb 10, 2017

(comment updated)

Hi
I've just struggled myself, but there is a more or less simple solution to this problem. Code:

// Some extra fields
// childKey is important! Otherwise o.title is used by default for <SearchResult> key
// and may lead to errors 
const mapToSearchResult = map(o => ({
      childKey: o.id,                                              //!!!!!
      as: IngredientsSearchResultWrapper,         //!!!!!
      id: o.id,
      title: o.title,
      image: o.picUrl,
      shortDesc: o.shortDescription || '',
      fullDesc: o.fullDescription || ''
    }))
const IngredientsSearchResultWrapper = (params) => (
  <div className={params.className} onClick={e => params.onClick(e)}>
    {params.children}
  </div>
)

const IngredientsSearchResult = ({id, title, image, shortDesc, fullDesc}) => {
  const style = !!image ? {backgroundImage: `url(${ingredient_pic_search(image)})`} : {}

  return (
    <div key={id}>
      <div className="pic" style={style}></div>
      <div className="main">
        <div className="title">{title}</div>
        {!!shortDesc &&
          <div className="desc">{shortDesc}</div>
        }
      </div>
    </div>
  )
}

<Search
      resultRenderer={IngredientsSearchResult}
      onResultSelect = {onResultSelect}
      onSearchChange = {onSearchChange}
      results = {results}
      minCharacters = {0}
      fluid
      value = {value}
      open = {true}
    />

@xeno
Copy link

xeno commented Jun 15, 2017

This is the same issue as #1642 and will be resolved by #1599

@levithomason
Copy link
Member

Heads up, these are two separate issues. The issue here is in the logic of the result renderer itself. The issue fixed by #1599 deals strictly with the Modal component.

@mcroba
Copy link

mcroba commented Jul 21, 2017

Any news on this issue?

@levithomason
Copy link
Member

Doesn't appear so. Just waiting for a PR.

@mcroba
Copy link

mcroba commented Jul 31, 2017

@levithomason Thanks for your response but do you know if there is someone working on that?

@paul-ng
Copy link

paul-ng commented Aug 1, 2017

I don't see a pull request that address this. I have a question though: Isn't it neater to have SearchResult as a Higher Order Component and pass along all props to a the custom/default resultRenderer?

@mjasnikovs
Copy link

mjasnikovs commented Sep 26, 2017

Search component in the current state is too opinionated.
I think this could be an easy fix. Just allow pass throw for one custom prop. It should allow containing a custom object.

Additionally:

/** A unique identifier. */
id: PropTypes.number,

Em, why number? This should allow for string input to. For results from MongoDB where ID is a string.
I return string id from Postgres to, as numeric ID could grow out of javascript number size.

@noinkling
Copy link
Contributor

noinkling commented Oct 16, 2017

In my case I'm happy enough with the default renderer, but I'm having a related issue where I want to conditionally process the result in handleResultSelect based on:

  1. Some arbitrary extra data that I choose to attach (e.g. IDs of associated entities)
  2. Its category

If you include extra properties on result objects there isn't even a warning as of React 16 - they simply get added as attributes to the <div>, polluting the HTML.

My workaround at the moment is to repurpose the id property by turning it into an object for storing this information - but I think everyone would agree that that's far from ideal. To be honest even just an extra recognized prop (as @mjasnikovs suggests) named extra or something would be welcome, given that there seems to have been no real movement on this issue (and may not be for a while yet). Let me know if that would be a PR worth submitting (also I don't know how things work in regards to parity with vanilla SUI).

Additionally, for a category search you should be able to retrieve the category name and/or key for the result in handleResultSelect regardless, i.e. without having to manually include it or iterating through results to check where it belongs. I guess that's technically a separate issue, but it's worth bringing up.

@HemalR
Copy link

HemalR commented Dec 9, 2017

Is this a duplicate of: #1681 ?

@jahosh
Copy link

jahosh commented Dec 15, 2017

@imns how did you eventually get around this?

@imns
Copy link
Author

imns commented Dec 18, 2017

@josh91hickman I think I just ended up using something else. This was a year ago so I don't 100% remember to be honest.

@mariorodriguespt
Copy link

This issue is quite annoying for me as I've error on the console that sometimes break the UI flow like clicking on a button twice.

A workaround is to use data- attributes, you'll get this attributes in the second argument: https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes

@lednhatkhanh
Copy link

Any solutions for this?

@HemalR
Copy link

HemalR commented Jan 19, 2018

Not yet. However, from my slowly growing understanding of React, I think this evolution here mayy solve this particular issue: https://reactjs.org/blog/2017/11/28/react-v16.2.0-fragment-support.html

Thoughts, @levithomason ?

@levithomason
Copy link
Member

I don't use the search component. Someone from the community will need to step up for this one.

@chemic
Copy link

chemic commented May 10, 2018

Any luck with Pull Requests getting tested/merged? 😋

@gabrielruss
Copy link

Any updates?

@pztheron
Copy link

This seems like you have to jump through non-intuitive hoops to get something simple to work.

@martindale
Copy link

@imns in your original example, where is client coming from?

@imns
Copy link
Author

imns commented Sep 10, 2018

@martindale in this example it was from FeathersJS, but it could be any HTTP client though.

@tobloef
Copy link

tobloef commented Sep 19, 2018

I ended up creating my own search bar with an Input and a Popup component, but I really do agree that this should be fixed.

@stevecastaneda
Copy link

stevecastaneda commented Dec 12, 2018

I was able to resolve the issue by deleting the offending prop (userID, in my case) from the object in the results array. I wasn't using it in the click handler anyhow.

@hitendramalviya
Copy link

Why consumer of this component being force to use object schema predefined by author/contributor of this component, which is like {title, price, etc...} at-lease result renderer should be given free hand to decide whatever props we want to use to render the result.

@tom-wagner
Copy link

I agree with everyone in this thread -- determining what props are allowable, such as arbitrary values like description and price seems unreasonable. I mentioned this to the rest of my team during grooming and we had a good laugh.

However, it doesn't appear this is going to be fixed anytime soon. As such I moved forward with iamdanthedev's solution from above, with one small change. In the item I was rendering I included the property id as a prop, not key.

Hope this helps and I hope this gets fixed!!

@imns
Copy link
Author

imns commented Mar 5, 2019

Based on tom-wagner comment and iamdanthedev's suggest work around I am closing this.

@imns imns closed this as completed Mar 5, 2019
@JeromeDeLeon
Copy link

JeromeDeLeon commented May 4, 2019

Unknown props like other meta data of my search show on html as attribute and it is because of unhandled props, so what if you handle the props like create prop of "metadata" or "data" where we pass other property to that prop so it will not show in the DOM and pass that prop when selecting result.

Example of passing other props (although, no errors being shown, it is kinda awkward to see these attributes on the DOM):
<div hello="e" otherdata="321" class="active result"></div>

@botoxparty
Copy link

My solution for this is to add the object as the description as a JSON string.

e.g. result: { description: JSON.stringify(myobject), title: My Object, key: object1 }

and in my results renderer I convert it back to an object.

const parsed = JSON.parse(description)

@JeromeDeLeon
Copy link

description gets displayed so JSON string will be displayed as well

@botoxparty
Copy link

You need to parse it in your results renderer before it's displayed

@leftdevel
Copy link

No real fix for this at the moment (Sep 2019). Does anyone have thoughts about how should a PR looks like? Any guidelines? I could give it a try :)

@sameer-ingavale
Copy link

@iamdanthedev's code did not fix my error. still get 'Invalid prop results supplied to Search in my console. Even when I use the deafultRenderer I see the same error. Anyone else face this too?

Also, should I be worried about this issue breaking my app? Since everything is rendering just fine, am i missing something here?

@mazer-rakham
Copy link

So there is no fix for this still?
My code is below:

import React, { useState, useCallback, useEffect } from "react";
import { connect } from "react-redux";
import { debounce, throttle } from "lodash";
import { Container, Search, Label } from "semantic-ui-react";
import { lookupUserProfile } from "../../../Redux/Actions/profileActions";
const resultRenderer = ({ first_name, last_name }) => <Label content={first_name + " " + last_name} />
function AutoSuggest({ lookupUserProfile, userProfile }) {
  const [userQuery, setUserQuery] = useState("");

  const delayedQuery = useCallback(
    throttle(q => sendQuery(q), 500),
    []
  );

  const onChange = e => {
    setUserQuery(e.target.value);
    delayedQuery(e.target.value);
  };
  const sendQuery = query => lookupUserProfile(query);

  return (
    <Container>
      <h1>Autosuggest test</h1>
      <div>
        <Search
          results={userProfile}
          onSearchChange={onChange}
          value={userQuery}
          resultRenderer={resultRenderer}
          
        />
      </div>
    </Container>
  );
}
const mapStateToProps = state => ({
  isLoading: state.profile.isLoading,
  userProfile: state.profile.user_profile
});

export default connect(mapStateToProps, { lookupUserProfile })(AutoSuggest);`

@eprincen2
Copy link

I just deleted a rant. I appreciate all you do, but this Search component is WAY too opinionated. It should just allow simple custom rendering of arbitrary user data. I'm just going to write my own that doesn't include random key assignment and required titles and stuff like that. That seems to be what most people end up doing.

@chadwtkns
Copy link

@iamdanthedev where does the mapToSearchResult function go/ where does it get called?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests