-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
[RFR] Add warnings to translate HOC when used directly to translate a string #2318
Conversation
djhi
commented
Sep 13, 2018
@@ -1,6 +1,7 @@ | |||
import React, { Component } from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import wrapDisplayName from 'recompose/wrapDisplayName'; | |||
import warning from 'warning'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this package is internal to facebook (see https://github.com/facebook/fbjs), use console.warn()
instead, and please remove usage from the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a mirror of Facebook's (FB) warning module used within React's source code (and other FB software). As such this module will mirror their code as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remembered that I introduced this package when working on permissions. I wonder whether we should use it everywhere now. It only display warnings when not in production so that one check we don't have to verify ourselves ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Facebook removed fbjs from the React dependencies, and warned that it's an internal package. The mirror is just adding more confusion on top of that. For such a simple function (it's a one liner), I think we can avoid the (controversial) dependency.
@@ -21,6 +22,15 @@ import wrapDisplayName from 'recompose/wrapDisplayName'; | |||
* @param {*} BaseComponent The component to decorate | |||
*/ | |||
const translate = BaseComponent => { | |||
warning( | |||
typeof BaseComponent !== 'string', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about function components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are not string right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning displays its warning if the condition is not met. Here, if BaseComponent
is a string, then we'll have a warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning
component is backwards. Another reason not to use it.
One last demand: please open against |
… string - [x] Add warning to translate HOC - [x] Remove dependency to `warning`
343d1c9
to
03bfc9a
Compare