Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

Warn & ignore when receiving null or undefined values #250

Merged
merged 4 commits into from
Jul 13, 2015

Conversation

AnSavvides
Copy link
Contributor

Title says it all!

I also added a couple of tests and made a change to .eslintrc to allow the use of undefined since we need to compare against it & use it for testing.

Closes #245.

} else {
/* eslint-disable no-console */
if (console && console.warn) {
console.warn('CSS value is "' + value + '" for property "' + property + '"');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add the component name as in #253?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I might be missing something, but we don't have a reference to a component within prefixer? Should getPrefixedStyle be taking a component as an argument for the sake of adding the name in the warning?

@ianobermiller
Copy link
Contributor

Yeah true, that would be crummy. I also wonder if we should just use typeof value === 'undefined' instead of removing the lint rule, but I don't care too much, so merge it is!

ianobermiller added a commit that referenced this pull request Jul 13, 2015
Warn & ignore when receiving null or undefined values
@ianobermiller ianobermiller merged commit a1d3f8c into FormidableLabs:master Jul 13, 2015
@bobbyrenwick
Copy link
Contributor

@ianobermiller @AnSavvides I think it's really useful to have the component name when you're debugging these kinds of errors, as well as unsupported CSS values / properties.

What about moving the responsibility of console.warn to the caller of getPrefixedStyle and having getPrefixedStyle return errors to the caller in some way? That way we would know what component we were rendering at the time.

@AnSavvides
Copy link
Contributor Author

@bobbyrenwick if we really wanted to have the component name in the warning, I would favor just passing component as an argument over having the warning in the caller of getPrefixedStyle - I just think that's a little cleaner, maybe? Not too fussed in any case!

@ianobermiller if you think that's a good idea (either approach) feel free to create an issue, assign it to me and I'll happily sort it out :)

@ianobermiller
Copy link
Contributor

Good discussion. Let's pass the component name to the prefixer just to make the warnings better. Also, thoughts on enabling warnings only in DEV?

@AnSavvides
Copy link
Contributor Author

I would be in favor of enabling warnings only in DEV, I think that's good practice. If it makes any difference, that's how it's done in React too (here for example), so React developers should be used to only getting warnings in DEV.

@bobbyrenwick
Copy link
Contributor

I'm also a fan of only having them in dev. We can use the same approach I used for the hot loading fix.

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

Successfully merging this pull request may close these issues.

None yet

3 participants