warn consumers if themed styles are misused. fix #1005 #1006

iamstarkov · 2019-01-29T15:54:44Z

What would you like to add/fix?

I would like to get feedback on initial implementation and get clarification

Also I didn't get how can I achieve the same (NODE_ENV !== 'production') functionality with tiny-warning module. I didnt grasp how that is achievable from it's documentation.

If implementation is ok, I'll add tests.

Also I ran yarn as contributing document demands but eslint complains:

   ✖ eslint
🚫 eslint found some errors. Please fix them and try committing again.

/Users/vlasta/projects/oss/jss/packages/react-jss/src/withStyles.js
  4:64  error  Unable to resolve path to module 'jss'  import/no-unresolved

✖ 1 problem (1 error, 0 warnings)

system info for eslint bug:

~/projects/oss/jss feat/themed-styles-misuse-warning
☯ node -v && yarn -v
v10.14.2
1.13.0

Corresponding issue (if exists):
#1005

kof · 2019-01-29T15:57:39Z

packages/react-jss/src/withStyles.js

+      console.warn(
+        `
+        Warning: [JSS]: This Component uses themed styles notation (function) while not relying on a theme (0 arguments).
+        It slows your app down and you should rewrite these styles to plain object notation.


I would remove the slows down part, because one might interpret it as something that is really really slow, instead of "it could be a bit better, because styles are removed across the element". I think we can skip the explanation. Just give a hint what to do.

I disagree.

it could be a bit better

This part will give anyone free pass to not do any optimisations and people will complain to remove the warning.

styles are removed across the element

I am not sure I understand what does it mean.

kof · 2019-01-29T15:58:11Z

packages/react-jss/src/withStyles.js

@@ -39,6 +39,29 @@ const getStyles = <Theme: {}>(styles: Styles<Theme>, theme: Theme) => {
  if (typeof styles !== 'function') {
    return styles
  }
+  if (process.env.NODE_ENV !== 'production') {
+    if (styles.length < 1) {


I would check for === 1, so that it includes also more than one arguments, just in case

One generic message that covers too both cases is good enough, more than one argument is not something that many will have

that's the second check. I separated these two cases, because 1st one is perf concern for sure, the 2nd one is misuse concern of unknown origin

Yeah, I would make it one and write a message that covers both cases. Its not a huge problem we need to be particularly explicit about.

"Styles function expects one theme argument." or something

I would argue that warning messages should not only warn but also provide suggestions for better dx

I don't see why cant we provide specific warning explanations for each case, because it will be removed from prod build anyway

I think we should start adding links to the warnings with a really complete full explanation. An explanation that can't be complete is not much better than a really short one without an explanation at all in my opinion.

For example your explanation was confusing for me.

kof · 2019-01-29T15:59:14Z

packages/react-jss/src/withStyles.js

@@ -39,6 +39,29 @@ const getStyles = <Theme: {}>(styles: Styles<Theme>, theme: Theme) => {
  if (typeof styles !== 'function') {
    return styles
  }
+  if (process.env.NODE_ENV !== 'production') {
+    if (styles.length < 1) {
+      console.warn(


warning(false, 'error')

why false?

I thought it would be something `warning(process.env.NODE_ENV !== 'production' && styles.length < 1, 'ERROR')

no, it is designed to be used like this warning(isValueCorrect, 'error') so if value is correct no warning happens

like an assertion

this warning will be stripped away from prod build anyway, right?

Yeah, all of the warning calls are automatically wrapped with process.env.NODE_ENV === 'development'.

Just checked, yes it gets removed in the minified version, even if it isn't wrapped with env conditional. Not sure who removes it though :)

All of the warning come this in the bundled code:

if (process.env.NODE_ENV !== 'production') { warning(); }

In the minified umd version this will be removed because we replace the node env. In the commonjs and ESM, it will exist until the user bundles the code.

true, something like this

warning(false, "[JSS] Bad keyframes name " + key);

becomes in the ESM bundle

process.env.NODE_ENV !== "production" ? warning(false, "[JSS] Bad keyframes name " + key) : void 0;

so later uglify probably removes it once conditional becomes 'production' !== 'production'

kof · 2019-01-29T16:00:25Z

packages/react-jss/src/withStyles.js

+        Warning: [JSS]: This Component uses themed styles notation (function) while not relying on a theme (0 arguments).
+        It slows your app down and you should rewrite these styles to plain object notation.
+      `
+          .replace(/[\s]{2,}/gim, ' ')


Why do we need that regexp?

it is multiline message so it will keep all indentation when being printed without adjusting

I think this is something that should be evtl done in warning module

or keep errors short

kof · 2019-01-29T16:17:07Z

regarding the lint error, does it go away if you run yarn?

iamstarkov · 2019-01-30T09:00:16Z

regarding the lint error, does it go away if you run yarn?

no

iamstarkov · 2019-01-30T09:01:13Z

also should we pass the component name to the getStyles function to provide better contextual feedback?

iamstarkov · 2019-01-30T09:55:10Z

here is a complete log of "import/no-unresolved" violations https://pastebin.com/HfBhv3q1

HenriBeck · 2019-01-30T09:58:27Z

Run yarn build

HenriBeck · 2019-01-30T09:59:03Z

Also please update the changelog

iamstarkov · 2019-01-30T11:47:32Z

Run yarn build

@HenriBeck, this helped, thanks

kof · 2019-01-30T12:41:30Z

packages/react-jss/src/withStyles.js

-  }
+  warning(
+    styles.length < 1,
+    `Warning: [JSS]: <${displayName} />'s styles function doesn't rely on a theme. We recommend to rewrite it to plain object.\nRead more: https://github.com/cssinjs/jss/blob/master/docs/react-jss.md#basic`


what if the styles function is used with multiple components?

We have to discuss the format of the links we use and the content. I was actually thinking of separate documents, which explain exactly the error, not just generic usage documents. Basically, the way react does that. But its a bit out of scope for this issue.

Another reason for keeping those error messages urls separate is that we need to have something more robust than the generric documentation, because if it changes, we will have wrong linnks in code that is distributed for years.

We need links we are never going to touch.

that doesn't change the misuse scenario in one or all of the components.

in my experience (few thousands components) people do it only when dont have time to split big stylesheet into separate ones and keep adding styles in one stylesheet for several components.

yeah, could be good enough, I don't see a good other pointer except of the stack trace, fn.name won't be useful since its usually styles

Why would it be a problem if it's used multiple times?

Its just a bit of indirection, it might be used in multiple but will mention one. But its ok I guess.

We need to get rid of the links, I don't want to have issues complaining about broken links. We need permalinks and full issue descriptions.

If you don't mind I will change the error message, we have a separate issue for linking errors to some permlink with good deescriptions

HenriBeck · 2019-01-30T12:52:11Z

packages/react-jss/src/withStyles.js

  if (typeof styles !== 'function') {
    return styles
  }
+  warning(
+    styles.length < 1,


Let's check for === 0 here?

He is using 2 separate warnings for 2 cases, I would do one, because more than one arguments case is not really an issue ...

lets do one

upgrade karma-browserstack-launcher to at least 1.4.0, because "internalBinding is not defined" due to old "natives" module See more: * karma-runner/karma-browserstack-launcher#140 * popcodeorg/popcode#1626

iamstarkov · 2019-01-31T13:12:47Z

I think eslint on travis has the same problem as I did https://travis-ci.org/cssinjs/jss/builds/486923274?utm_source=github_status&utm_medium=notification

HenriBeck · 2019-01-31T14:00:39Z

Merge master in this branch again

iamstarkov · 2019-01-31T14:30:47Z

@HenriBeck didn't help

iamstarkov · 2019-01-31T14:33:36Z

also I have problems with the following test case. I forgot to add it from the start, and now am struggling with karma. I suspect karma isn't configured to propagate process variable into the karma env.

    it.only('doesnt warn if themed styles dont use theme in _prod_', () => {
      debugger;
      process.env.NODE_ENV = 'production'
      function DisplayNameTest() {
        return null
      }
      const MyComponent = withStyles(() => ({}))(DisplayNameTest) // eslint-disable-line no-unused-vars

      TestRenderer.create(<MyComponent />)

      expect(console.warn.called).to.be(false)
      process.env.NODE_ENV = 'development'
    })

HenriBeck · 2019-01-31T15:35:38Z

@iamstarkov try now again, forgot that I hadn't merged the PR which fixes it yet.

iamstarkov · 2019-02-01T17:49:54Z

it works now, but aforementioned test case isn't passing. Can someone else take a look at it?

HenriBeck · 2019-02-01T20:17:13Z

Will have a look tomorrow.

HenriBeck · 2019-02-02T00:08:21Z

@iamstarkov the test is passing for me locally, is the test still a problem?

iamstarkov · 2019-02-02T20:05:06Z

@HenriBeck it doesn't pass for me locally. But I committed it now, lets see what travis thinks

HenriBeck · 2019-02-02T20:06:08Z

Ohh, I thought you already committed it. Will have a look again

HenriBeck · 2019-02-02T23:13:42Z

I fixed the test, though a different test is now failing, not sure why though.

…g' into feat/themed-styles-misuse-warning * origin/feat/themed-styles-misuse-warning: [react-jss] Export the context as __Context from react-jss (#1014) [jss-plugin-camel-case] Support css vars better (#1017) [docs] Add badges to readmes (#1016) Revert "Add tests for css variables in camel case plugins" Revert "Fix css variables being hyphenated" Revert "Run prettier" Run prettier Fix css variables being hyphenated Add tests for css variables in camel case plugins

HenriBeck · 2019-02-03T13:02:52Z

Is this good to merge?

iamstarkov · 2019-02-03T14:03:02Z

@HenriBeck from the implementation point of view it is good to merge

HenriBeck · 2019-02-03T14:08:47Z

Alright, merging it then. I don't think we need to add any kind of documentation for it.

…te-sheets-manager * 'master' of https://github.com/cssinjs/jss: warn consumers if themed styles are misused. fix #1005 (#1006) Added TypeScript Usage documentation for React JSS (#1009)

iamstarkov · 2019-02-04T10:40:45Z

yay

kof · 2019-02-04T10:41:31Z

I am removing the link though.

…1006

iamstarkov · 2019-02-06T13:41:26Z

ok

… update-jss-types * jss-plugin-rule-value-function/fix-process-styles: (35 commits) Add formatting TS files Update typings Use types jss definitions Fix TS type definitions (#1030) v10.0.0-alpha.10 Update publish scripts (#1027) [docs] Update SSR setup with react-jss (#1018) temporarily remove the link to the docs, we need perma links instead #1006 [jss] Update SheetsManager (#1019) warn consumers if themed styles are misused. fix #1005 (#1006) Added TypeScript Usage documentation for React JSS (#1009) [react-jss] Export the context as __Context from react-jss (#1014) [jss-plugin-camel-case] Support css vars better (#1017) [docs] Add badges to readmes (#1016) Revert "Add tests for css variables in camel case plugins" Revert "Fix css variables being hyphenated" Revert "Run prettier" Run prettier Fix css variables being hyphenated Add tests for css variables in camel case plugins ... # Conflicts: # packages/jss/src/index.d.ts

…ssinjs#1006) * warn consumers if themed styles are misused. fix cssinjs#1005 * Fix tests on node 10 upgrade karma-browserstack-launcher to at least 1.4.0, because "internalBinding is not defined" due to old "natives" module See more: * karma-runner/karma-browserstack-launcher#140 * popcodeorg/popcode#1626 * add tests * update snapshots * add test for themed styles misuse warning not shown in prod * Add dev expression babel plugin while testing * Remove test and babel plugin from test setup * Update size-snapshot

…nstead cssinjs#1006

iamstarkov requested a review from kof January 29, 2019 15:56

kof reviewed Jan 29, 2019

View reviewed changes

kof reviewed Jan 30, 2019

View reviewed changes

HenriBeck reviewed Jan 30, 2019

View reviewed changes

HenriBeck approved these changes Jan 30, 2019

View reviewed changes

iamstarkov added 4 commits January 31, 2019 13:51

warn consumers if themed styles are misused. fix #1005

4cc1970

Fix tests on node 10

e740e2f

upgrade karma-browserstack-launcher to at least 1.4.0, because "internalBinding is not defined" due to old "natives" module See more: * karma-runner/karma-browserstack-launcher#140 * popcodeorg/popcode#1626

add tests

6695c88

update snapshots

bc5a50c

iamstarkov force-pushed the feat/themed-styles-misuse-warning branch from 72dea2c to bc5a50c Compare January 31, 2019 13:02

Merge branch 'master' into feat/themed-styles-misuse-warning

42ef705

Merge branch 'master' into feat/themed-styles-misuse-warning

dfcad3b

add test for themed styles misuse warning not shown in prod

4b34d68

Add dev expression babel plugin while testing

41b9729

Henri added 4 commits February 3, 2019 00:19

Merge branch 'master' into feat/themed-styles-misuse-warning

08054bd

Remove test and babel plugin from test setup

9eaa13c

Update size-snapshot

56800f9

HenriBeck merged commit 5e80923 into master Feb 3, 2019

HenriBeck deleted the feat/themed-styles-misuse-warning branch February 3, 2019 14:08

kof added a commit that referenced this pull request Feb 4, 2019

temporarily remove the link to the docs, we need perma links instead #…

019ce01

…1006

kof added a commit that referenced this pull request Feb 4, 2019

temporarily remove the link to the docs, we need perma links instead #…

9cc1399

…1006

bhupinderbola pushed a commit to bhupinderbola/jss that referenced this pull request Sep 17, 2019

(split)temporarily remove the link to the docs, we need perma links i…

1c6bae2

…nstead cssinjs#1006

warn consumers if themed styles are misused. fix #1005 #1006

warn consumers if themed styles are misused. fix #1005 #1006

Conversation

iamstarkov commented Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kof Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kof Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kof commented Jan 29, 2019

iamstarkov commented Jan 30, 2019

iamstarkov commented Jan 30, 2019

iamstarkov commented Jan 30, 2019

HenriBeck commented Jan 30, 2019

HenriBeck commented Jan 30, 2019

iamstarkov commented Jan 30, 2019

Choose a reason for hiding this comment

kof Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamstarkov commented Jan 31, 2019

HenriBeck commented Jan 31, 2019

iamstarkov commented Jan 31, 2019

iamstarkov commented Jan 31, 2019

HenriBeck commented Jan 31, 2019

iamstarkov commented Feb 1, 2019

HenriBeck commented Feb 1, 2019

HenriBeck commented Feb 2, 2019

iamstarkov commented Feb 2, 2019

HenriBeck commented Feb 2, 2019

HenriBeck commented Feb 2, 2019

HenriBeck commented Feb 3, 2019

iamstarkov commented Feb 3, 2019

HenriBeck commented Feb 3, 2019

iamstarkov commented Feb 4, 2019

kof commented Feb 4, 2019

iamstarkov commented Feb 6, 2019

iamstarkov commented Jan 29, 2019 •

edited

Loading

kof Jan 29, 2019 •

edited

Loading

kof Jan 30, 2019 •

edited

Loading

kof Jan 30, 2019 •

edited

Loading