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 "Unknown props" warnings #64

Closed
RSchwan opened this issue Jul 6, 2016 · 34 comments
Closed

React "Unknown props" warnings #64

RSchwan opened this issue Jul 6, 2016 · 34 comments
Assignees
Labels
Type: Bug Bug reports and their fixes

Comments

@RSchwan
Copy link
Contributor

RSchwan commented Jul 6, 2016

I'm not sure if this is intended or not but I get a bunch of "Unknown props" errors.
Per example:

Warning: Unknown props `model`, `schema`, `validate` on <form> tag. Remove these props from the element. For details, see https://fb.me/react-unknown-prop
    in form
    in AutoValidatedQuickSemanticForm (created by CreateDivisionPage)
    in div (created by CreateDivisionPage)
    in div (created by Modal)
    in div (created by Modal)
    in div (created by Modal)
    in Modal (created by CreateDivisionPage)
    in CreateDivisionPage (created by RouterContext)
    in div (created by AppLayout)
@radekmie
Copy link
Contributor

radekmie commented Jul 6, 2016

It's a new feature in React@15.2.0 - it's on my roadmap, but it will take a long time to filter all props. I'll do it step by step, but every help is appreciated.

@radekmie radekmie added the Type: Bug Bug reports and their fixes label Jul 6, 2016
@radekmie radekmie self-assigned this Jul 6, 2016
@RSchwan
Copy link
Contributor Author

RSchwan commented Jul 6, 2016

I could help. I just need to know how so that everything is done the same way.

@radekmie
Copy link
Contributor

radekmie commented Jul 6, 2016

Okay - I'll do it in one of the packages.

@radekmie radekmie changed the title React "Unknown props" errors React "Unknown props" warnings Jul 6, 2016
@serkandurusoy
Copy link
Contributor

@radekmie this is the list of standard dom properties that are supported by react

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/HTMLDOMPropertyConfig.js#L24

Do you think we can create a wrapper around this that shares a props object to two different objects, one with the allowed standard ones and other with the remaining, custom ones.

this way, we can pass on the second one as props and use the first one where appropriate

@serkandurusoy
Copy link
Contributor

For reference, here's a proper Array:

const ALLOWED_STANDARD_HTML_PROPERTIES = [
  "accept",
  "acceptCharset",
  "accessKey",
  "action",
  "allowFullScreen",
  "allowTransparency",
  "alt",
  "async",
  "autoComplete",
  "autoPlay",
  "capture",
  "cellPadding",
  "cellSpacing",
  "charSet",
  "challenge",
  "checked",
  "cite",
  "classID",
  "className",
  "cols",
  "colSpan",
  "content",
  "contentEditable",
  "contextMenu",
  "controls",
  "coords",
  "crossOrigin",
  "data",
  "dateTime",
  "default",
  "defer",
  "dir",
  "disabled",
  "download",
  "draggable",
  "encType",
  "form",
  "formAction",
  "formEncType",
  "formMethod",
  "formNoValidate",
  "formTarget",
  "frameBorder",
  "headers",
  "height",
  "hidden",
  "high",
  "href",
  "hrefLang",
  "htmlFor",
  "httpEquiv",
  "icon",
  "id",
  "inputMode",
  "integrity",
  "is",
  "keyParams",
  "keyType",
  "kind",
  "label",
  "lang",
  "list",
  "loop",
  "low",
  "manifest",
  "marginHeight",
  "marginWidth",
  "max",
  "maxLength",
  "media",
  "mediaGroup",
  "method",
  "min",
  "minLength",
  "multiple",
  "muted",
  "name",
  "nonce",
  "noValidate",
  "open",
  "optimum",
  "pattern",
  "placeholder",
  "poster",
  "preload",
  "profile",
  "radioGroup",
  "readOnly",
  "rel",
  "required",
  "reversed",
  "role",
  "rows",
  "rowSpan",
  "sandbox",
  "scope",
  "scoped",
  "scrolling",
  "seamless",
  "selected",
  "shape",
  "size",
  "sizes",
  "span",
  "spellCheck",
  "src",
  "srcDoc",
  "srcLang",
  "srcSet",
  "start",
  "step",
  "style",
  "summary",
  "tabIndex",
  "target",
  "title",
  "type",
  "useMap",
  "value",
  "width",
  "wmode",
  "wrap",
  "about",
  "datatype",
  "inlist",
  "prefix",
  "property",
  "resource",
  "typeof",
  "vocab",
  "autoCapitalize",
  "autoCorrect",
  "autoSave",
  "color",
  "itemProp",
  "itemScope",
  "itemType",
  "itemID",
  "itemRef",
  "results",
  "security",
  "unselectable",
];

And we should also whitelist and send down all props that begin with data- or aria-

@serkandurusoy
Copy link
Contributor

Apart from the unknown props, there other other erros:

Failed prop type: AutoValidatedQuickUnstyledForm: prop type `onChangeModel` is invalid; it must be a function, usually from React.PropTypes.
Unknown DOM property autosave. Did you mean autoSave?
Unknown props `model`, `schema`, `validate` on <form> tag. Remove these props from the element. For details, see https://fb.me/react-unknown-prop

@radekmie
Copy link
Contributor

radekmie commented Jul 7, 2016

I think it's unnecessary @serkandurusoy - I've done it manually in uniforms-bootstrap3 - it wasn't that hard and long.

  1. It's already done.
  2. It's also already done.
  3. And this one too.

@sabativi
Copy link

sabativi commented Jul 9, 2016

I am right saying that it is not working because of those errors with react 15.0.2 ?

I can't managed to have your simple example working...

@radekmie
Copy link
Contributor

radekmie commented Jul 9, 2016

These are just warnings introduced in react@15.2.0. More info here. If you need some help, @sabativi, then go ahead and write me an email or create an issue if you've encountered a (possible) bug.

@radekmie
Copy link
Contributor

Okay, I think I've removed everything - comment here if you find something left.

@serkandurusoy
Copy link
Contributor

Seems like fixed, thanks!

@MacRusher
Copy link
Member

Missed one:

Warning: Unknown prop `regEx` on <section> tag. Remove this prop from the element.
  in section (created by Text)
  in Text (created by BaseField(Text))

Happens only when you have a regEx key in the schema.

I guess that other custom SimpleSchema extensions would also be passed down.

@RSchwan
Copy link
Contributor Author

RSchwan commented Jul 11, 2016

You also missed the custom key:

Warning: Unknown prop `custom` on <section> tag. Remove this prop from the element. For details, see https://fb.me/react-unknown-prop
    in section (created by Num)
    in Num (created by BaseField(Num))
    in BaseField(Num) (created by Auto)
    in Auto (created by BaseField(Auto))
    in BaseField(Auto) (created by DivisionForm)

@radekmie
Copy link
Contributor

Well, those are from SimpleSchema, so I don't really know, what to do - it won't be a good idea to intercept every schema props from every field... I guess, that we have to go along with something like @serkandurusoy have suggested. I'll try to come up with something, but ideas are welcome (as always).

@radekmie radekmie reopened this Jul 11, 2016
@MacRusher
Copy link
Member

Maybe SimpleSchema related props could be handled on a bridge-level, maybe only limited whitelisted props could be passed down to components?

Other then that only solution I see is to filter out invalid values before passing them to elements.

@serkandurusoy
Copy link
Contributor

Simple schema options can be extended in which case whitelisting fields on
SS would cause custom options to cause problems.

for example, uniforms is a custom ss property!

On Mon, Jul 11, 2016 at 7:50 PM, Maciek Stasiełuk notifications@github.com
wrote:

Maybe SimpleSchema related props could be handled on a bridge-level, maybe
only limited whitelisted props could be passed down to components?

Other then that only solution I see is to filter out invalid values before
passing them to elements.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#64 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AEbz3KPVnraRbXIL54axSKFVvREEwmkCks5qUnRcgaJpZM4JGCcd
.

@radekmie
Copy link
Contributor

I think that something like:

export function filterDOMProps (props) {
    // Either
    props = {...props};
    // or
    props = Object.assign({}, props);

    delete props.findError;
    delete props.findField;
    delete props.findType;
    // ...

    return props;
}

in core package will be sufficient. What do you think?

@radekmie
Copy link
Contributor

@serkandurusoy
Copy link
Contributor

You will then need to maintain this and also anyone extending current components will also need to be aware of that.

So instead, I think you should go the other way around. Filter out react's "known props" (the list I provided in an earlier comment) and do a return { standardProps, customProps } instead of return props

@MacRusher
Copy link
Member

MacRusher commented Jul 11, 2016

I was thinking more about something like:

export function filterDOMProps (props) {
  const DOMProps = {};
  for (let prop in props) {
    if (
      prop.startsWith('data-') ||
      prop.startsWith('aria-') ||
      ALLOWED_STANDARD_HTML_PROPERTIES.includes(prop)
    ) {
      DOMProps[prop] = props[prop];
    }
  }
  return DOMProps;
}

Of course some testing and benchmarks would be nice to have.

@radekmie
Copy link
Contributor

Yes, @MacRusher - that would be quite good, but some benchmarking is needed (that standard props array is big!). It's interesting, that React doesn't offer such function out of box.

@MacRusher
Copy link
Member

If we could write this in a way that browsers could be able to inline cache then this wouldn't be much of a problem, but I'm not sure at the moment if this is possible.
I can only imagine that almost all React-based libraries face the same issue right now so someone must find an effective solution :)

@serkandurusoy
Copy link
Contributor

Perhaps we could use a memoizing function to ease the re-evaluation?

@radekmie
Copy link
Contributor

Memoization would help, if we will call it many times with same objects - we won't.

@radekmie
Copy link
Contributor

Well, well, well.

@serkandurusoy
Copy link
Contributor

it would at least help for apps that create the same components over and over again, would it not?

And wow! So we go back to square one :)

@radekmie
Copy link
Contributor

Closing time!

I've added new filterDOMProps helper to the core package. By default, it remove all unwated props and has an option to register new ones through filterDOMProps.register(...props) (bridges will do it surely). If any props are left, please let me know or create a PR for it.

@radekmie
Copy link
Contributor

And here is a little gem I've found during benchmarks.

Browser no. 1 no. 2 no. 3
Chrome 53 49,532 101,640 108,928
Epiphany 3.20 88,915 50,065 71,810
Firefox 47 24,939 33,386 71,325
Opera 38 49,394 96,065 78,075
Vivaldi 1.2.490 49,376 95,817 81,536

Currently implemented is no. 3.


Why Firefox is so slow?

@BudgieInWA
Copy link
Contributor

This seems to be the wrong way to go about this. My reasoning is like this:

  1. Schema definitions are conceptually arbitrary objects that the schema understands.
  2. The bridge translates the schema definition into a standard definition that uniforms uses.
  3. The uniforms option of this standard definition is used to communicate Uniforms specific stuff.
  4. Attributes on the html element used to render the form control for a field are Uniforms specific stuff.
  5. So, the properties of my schema definition should not be used as attributes to the html element, instead, I should specify such attributes as part of the uniforms property in my definition.

To elaborate, even though a property of the standard definition, like min, have an equivalent html attribute, the Schema Bridge doesn't include a min property for this reason, but instead because it's a property that Uniforms expects. This is shown by the fact that in the case of a DateField, the type of the min property is different to the corresponding type of the underlying HTML element. The schema definition is so far removed from the attributes of an html element that collects a value, that it is wrong (IMO) to assume that a definition property should be an html attribute.

I would expect, instead, for html attributes to be specified in the uniforms key of my schema definition, maybe uniforms.htmlAttributes or uniforms.attrs or any property of uniforms that doesn't have any other meaning.

@radekmie
Copy link
Contributor

@BudgieInWA: sorry for the delay - I’m sure I’ve answered you but this comment seem to disappear.

It makes more or less sense, depending on the schema and your background. If you come from Meteor Blaze autoform, then you integrate schema validation almost 1 to 1 with form attributes. Other thing is that it was somehow a design decision to make it as easy to start as possible and let users tweak it to their needs. Right now, you can change this behavior by explicitly setting some values to undefined. I’m a bit unsure on what’s better - to double definition of max, min and others (which is IMO more common) than sometimes null it when needed.

Remember, that it’s also possible to implement own schema bridge. You can easily fork default ones to work with your case and separate them.

@michaellill-corefihub
Copy link

We had this problem when extending simpleschema. We solved it like this:

const schmemaExtensions = ["interfieldvisibility"];

SimpleSchema.extendOptions(schmemaExtensions);

if (Meteor.isClient) {
    schmemaExtensions.forEach((schmemaExtension) => {
        filterDOMProps.register(schmemaExtension as any);
    });
}

@michaellill-corefihub
Copy link

@radekmie may make sense to add this to the documentation of simpleschema bridge?

@radekmie
Copy link
Contributor

@michaellill-corefihub: Thanks for the suggestion! I believe we could just link to filterDOMProps in the bridges docs + extend the filterDOMProps docs. I'll file an issue for that.

@michaellill-corefihub
Copy link

@radekmie Even better. Thank you!

@Monteth Monteth added this to Closed in Open Source (migrated) Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Bug reports and their fixes
Projects
Archived in project
Development

No branches or pull requests

7 participants