Added postProcessCssNamespace to define different strategy to wrap cl… #15

daniloster · 2018-07-24T00:55:55Z

Hi guys, I had look at this issue #12 and added a new possibility with this PR. Added a feature to set multiple wrappers which create nested & {/* ... */} when required only. This new strategy should sort the problem mentioned in the issue, besides it conserves whatever the developer has added there as class definition by only wrapping the end result, almost like a post css processor.

I have seen that currently the array is treated as a single wrapper in the cssNamespace creating nested specificity. But, what if we use the concept of array as multiple wrappers and add the wrapper basically as a post css processor? It would be a matter of decorating the function that writes down the css classes and attributes.

I am sorry if this suggestion sounds stupid somehow.

The main reason for that is that the cssNamespace should be something that developer can set as e.g. cssNamespace: ['#root-div .wrapperClass', 'body.theme-whatever'] and this should generate wrappers like e.g. #root-div .wrapperClass, body.theme-whatever { /* whatever rules applied there */ }
At the moment the developer is setting this configuration, he knows exactly what are the wrapper required and there is no need to build this internally reducing the flexibility of this tools.

So, I have added an extra property to the configuration postProcessCssNamespace, then, the current behaviour is maintained (being not a breaking change) and developers can migrate or be ready to a future release where the prop postProcessCssNamespace takes place of cssNamespace being the main strategy.

I hope this change helps this library and any recommendations/critics are more than welcome.

Thank you

…ass definitions

nvanselow · 2018-07-24T04:49:39Z

This sounds great. Thanks very much for your contribution and thorough explanation. At first glance, your proposal sounds like a good way to enhance this. I will try to take a closer look tomorrow at the changes you are proposing. I’ve very much wanted to get back to improving this tool, but have been short on bandwidth.

daniloster · 2018-07-24T15:13:45Z

Hi all, I have noticed that I will need to update the docs if this proposal gets accepted. So, let me know and I will update it. Also, if you guys want to simply remove the previous behaviour that would be a breaking change release. I would not mind doing it. 😉

nvanselow · 2018-07-25T04:48:00Z

Thanks again for your contribution. I didn't get to this until late at night and started taking a look. I need a bit more time with it and hope to do that tomorrow.

Regarding the docs: It definitely makes sense to hold off until we are sure about that changes. We can add that at the end or do a fast-follow PR.

nvanselow

This is excellent. It took me a bit to get my head wrapped around what was happening because the solution is a bit different than the one I was mulling over in my head. I think I like your approach better!

I left a few minor comments. Honestly, they are fairly optional. Feel free to disagree.

If you'd like, please add the new doc to this PR. Otherwise, we can add it in a follow up PR.

Thanks again for the contribution!

nvanselow · 2018-07-26T04:38:18Z

src/visitors/cssNamespace.js

@@ -1,21 +1,49 @@
 import * as t from 'babel-types';
 import { isStyled } from 'babel-plugin-styled-components/lib/utils/detectors';

+const containsInitialSelfReference = css =>
+  /^(([^\{]([\w-_,:+\s]+))*\&\s*([^\{]([\w-_,:+\s]+))*)+[\{]/g.test(css.trim());


Regex is always tough to read and this bit is somewhat essential to how this works, can you please add some comments here that helps explain this regex a bit?

Something like: This regex checks to see if the css contains a self reference (&) at the beginning which is not proceeded by other valid css. For example, {someString} would return true but {someOtherString} would return false.

As we don't have tests for this particular function in isolation, I think those comments will help us remember what is going on when we look back on this.

I don't mind adding tests for the particular function. I believe it will improve coverage around the behaviour adopted. Btw, shall I add the new behaviour as it is in this PR (non-breaking change)? Or should we replace the current behaviour by this bew one and release as a breaking change?

nvanselow · 2018-07-26T04:39:17Z

src/visitors/cssNamespace.js

-  const { cssNamespace } = state.opts;
-  if (!cssNamespace) {
-    return '&&';
+  const { cssNamespace, postProcessCssNamespace } = state.opts;


I agree with your approach here. Let's leave this as a new option for now to avoid the breaking change. Once we have it in the wild for a bit and continue to confirm this works in more complex codebases, let's update to only have this option.

Haaa, I think you just answered my previous question. 😉

nvanselow · 2018-07-26T04:41:32Z

src/visitors/cssNamespace.js

-  if (Array.isArray(cssNamespace)) {
-    return `.${cssNamespace.join(' .')} &`;
+  if (cssNamespace) {
+    const wrapperClass = `.${[].concat(cssNamespace).join(' .')} &`;


Nice trick here! I'll have to remember this one. The .concat works with a string or array.

Concat can add a new element or each item of given array. 😁 Not really good trick for production if it is all the time running on client and creates new instances which can potentially invoke the garbage collector often and the degrade performance. But, here we do it on compile time which is ok. 😉

nvanselow · 2018-07-26T04:44:10Z

src/tests/__snapshots__/cssNamespace.test.js.snap

+export default MyStyledComponent;
+"
+`;
+


Excellent. I like how you've used examples right from the issue for the tests.

Can we add one additional test that explicitly checks for & + & as that was one other case noted in the issue?

nvanselow · 2018-07-26T04:46:30Z

src/visitors/cssNamespace.js

  }

-  return `.${cssNamespace} &`;
+  if (postProcessCssNamespace) {
+    const wrapperClass = `${[].concat(postProcessCssNamespace).join(', ')}`;


It looks like line 22 is the only difference between the cssNamespace and postProcessCssNamespace. Can/should we further reduce code duplication by only having this single line of code inside of the condition, then the rest of the lines below the conditional?

nvanselow · 2018-07-26T05:03:57Z

src/visitors/cssNamespace.js

+    : `${cssNamespace} {&{${css}${shouldLeaveScopeOpen ? '' : '}}'}`;
+
+const NO_STRIP = false;
+const STRIP_END_CURLY_BRACKETS = true;


I'm a fan of this pattern to make booleans as arguments a bit more readable. Very nice!

daniloster · 2018-07-26T08:53:10Z

Thanks so much for reviewing this. I will put the changes as soon as possible. 👍🎉

daniloster · 2018-07-29T11:25:23Z

@nvanselow I have added the changes as agreed. Every change is in a different commit to help the review. I added documentation, but, I did not add the version bump. I believe it is a minor change according to semver as it is adding a new feature.

I will leave up to you to decide it and release it, if you don't mind.

Thanks again!

nvanselow

Awesome work! These changes are right on. I like the new name as well.

I agree that this would be a minor version change at this point until we remove the original approach. I can get that published after we merge this PR.

daniloster · 2018-07-30T22:18:45Z

So, I have added the version bump as minor. Whenever suits you, could you merge and release it? Thank you

nvanselow · 2018-07-30T22:24:12Z

Will do! I will likely get this released first thing tomorrow. Thanks again!

daniloster · 2018-07-30T22:24:23Z

I think my last commit dismissed your approval 😆
Sorry

nvanselow

No worries! I appreciate the separate commits. Makes it easy to see what changed very quickly and re-approve.

nvanselow · 2018-07-31T16:04:03Z

Updating the package on NPM failed a few times. It looks like the api keys on NPM have changed. I am working on fixing this and hope to have this deployed soon to NPM.

daniloster · 2018-07-31T21:09:46Z

Just to let you know, a few weeks ago the all NPM tokens have been revoked due to a malicious code in 2 eslint plugins. It might be that the reason. 😉

nvanselow · 2018-07-31T21:11:56Z

Thanks for the heads up @daniloster. That was my line of thinking as well. I'm waiting on the owners of the npm org that hosts this package to provide me with an updated key. Hopefully won't be too much longer.

nvanselow · 2018-07-31T22:53:16Z

We got the new API key and this is live on NPM!

Added postProcessCssNamespace to define different strategy to wrap cl…

ccfb843

…ass definitions

daniloster requested review from genedwards, harrismd, mattschweers and nvanselow as code owners July 24, 2018 00:55

nvanselow reviewed Jul 26, 2018

View reviewed changes

daniloster added 3 commits July 29, 2018 11:34

Renamed postProcessCssNamespace to rawCssNamespace

dded781

Added comment explaining regex and renamed it

86183de

Refactored tests descriptions and added extra case for the bug mentioned

6bd87df

Added documentation about new behaviour

4198707

daniloster force-pushed the ADDING-FLEXIBLE-WRAPPERS branch from 51dd691 to 4198707 Compare July 29, 2018 11:31

nvanselow previously approved these changes Jul 30, 2018

View reviewed changes

Bumped the version to 0.1.0 as minor release

e024b52

daniloster dismissed nvanselow’s stale review via e024b52 July 30, 2018 22:17

nvanselow approved these changes Jul 30, 2018

View reviewed changes

nvanselow merged commit 7341ea4 into QuickBase:master Jul 31, 2018

nvanselow mentioned this pull request Jul 31, 2018

Namespace break adjacent sibling combinator selector #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added postProcessCssNamespace to define different strategy to wrap cl… #15

Added postProcessCssNamespace to define different strategy to wrap cl… #15

daniloster commented Jul 24, 2018 •

edited

Loading

nvanselow commented Jul 24, 2018

daniloster commented Jul 24, 2018 •

edited

Loading

nvanselow commented Jul 25, 2018

nvanselow left a comment

nvanselow Jul 26, 2018

daniloster Jul 26, 2018

nvanselow Jul 26, 2018

daniloster Jul 26, 2018

nvanselow Jul 26, 2018

daniloster Jul 26, 2018

nvanselow Jul 26, 2018

nvanselow Jul 26, 2018

nvanselow Jul 26, 2018

daniloster commented Jul 26, 2018 •

edited

Loading

daniloster commented Jul 29, 2018

nvanselow left a comment

daniloster commented Jul 30, 2018

nvanselow commented Jul 30, 2018

daniloster commented Jul 30, 2018

nvanselow left a comment

nvanselow commented Jul 31, 2018

daniloster commented Jul 31, 2018

nvanselow commented Jul 31, 2018

nvanselow commented Jul 31, 2018

Added postProcessCssNamespace to define different strategy to wrap cl… #15

Added postProcessCssNamespace to define different strategy to wrap cl… #15

Conversation

daniloster commented Jul 24, 2018 • edited Loading

nvanselow commented Jul 24, 2018

daniloster commented Jul 24, 2018 • edited Loading

nvanselow commented Jul 25, 2018

nvanselow left a 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

daniloster commented Jul 26, 2018 • edited Loading

daniloster commented Jul 29, 2018

nvanselow left a comment

Choose a reason for hiding this comment

daniloster commented Jul 30, 2018

nvanselow commented Jul 30, 2018

daniloster commented Jul 30, 2018

nvanselow left a comment

Choose a reason for hiding this comment

nvanselow commented Jul 31, 2018

daniloster commented Jul 31, 2018

nvanselow commented Jul 31, 2018

nvanselow commented Jul 31, 2018

daniloster commented Jul 24, 2018 •

edited

Loading

daniloster commented Jul 24, 2018 •

edited

Loading

daniloster commented Jul 26, 2018 •

edited

Loading