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

Add warning when using a function value inside a function rule #1285

Merged
merged 3 commits into from
Feb 26, 2020

Conversation

m4theushw
Copy link
Contributor

Corresponding issue (if exists):

#1057

What would you like to add/fix?

Add warning when user tries to use a function value inside a function rule.

Todo

  • Add tests if possible
  • Add changelog if users should know about the change
  • Add documentation

@@ -55,6 +56,13 @@ export default function functionPlugin() {
// Empty object will remove all currently defined props
// in case function rule returns a falsy value.
styleRule.style = fnRule(data) || {}

for (const prop in styleRule.style) {
Copy link
Member

Choose a reason for hiding this comment

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

We could avoid doing these checks in production by wrapping it in if (process.env.NODE_ENV === 'development')

Copy link
Member

Choose a reason for hiding this comment

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

Then we could even avoid warning package and use only console.warn.

Copy link
Member

Choose a reason for hiding this comment

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

I would keep warning for consistency, but it doesn't matter technically, just in case in the future we want to replace warning, it will be easier if it's used consistently

Copy link
Member

@kof kof left a comment

Choose a reason for hiding this comment

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

Just one thing otherwise looks good 👍

@m4theushw
Copy link
Contributor Author

@kof updated! I had to change webpack.config.js because process.env.NODE_ENV was undefined.

@kof kof merged commit d8e7622 into cssinjs:master Feb 26, 2020
@kof
Copy link
Member

kof commented Feb 26, 2020

LGTM, thank you!

@m4theushw m4theushw deleted the warning branch February 26, 2020 23:36
@kof
Copy link
Member

kof commented Mar 15, 2020

released in 10.1.0

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

Successfully merging this pull request may close these issues.

None yet

3 participants