refactor: reduce code complexity for css generation [SPA-2573] #999
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Chaoste
changed the title
Chaoste
changed the title
Chaoste
force-pushed
the
style/refactor-css-generation
branch
from
7afd5af to
324b16b
Compare
packages/core/src/utils/styleUtils/ssrStylesDesignTokenResolution.spec.tsx
Outdated
Show resolved
Hide resolved
| expect(styles).toBe( | ||
| '.cf-ae92b9eb2c20a262573a8085db3dbd67{box-sizing:border-box;margin:2rem;padding:2rem;background-color:white;width:50%;height:25%;border:1px solid black;gap:2rem 2rem;font-size:1rem;color:black;}@media(max-width:992px){.cf-1fe22311d2b832dd529a9ce5e8d77281{box-sizing:border-box;margin:1.5rem;padding:1.5rem;background-color:black;width:75%;height:50%;border:3px solid black;gap:1.5rem 1.5rem;font-size:0.75rem;color:green;}}@media(max-width:576px){.cf-63009ce9ca644a23e001721d6358e491{box-sizing:border-box;margin:1rem;padding:1rem;background-color:red;width:100%;height:100%;border:0px solid transparent;gap:1rem 1rem;font-size:1.5rem;color:orange;}}', | ||
| expect(styles).toMatchInlineSnapshot( | ||
| `".cf-7b1d28fe46d9488fa595e4ff21270f2e{box-sizing:border-box;margin:2rem;padding:2rem;background-color:white;width:50%;height:25%;border:1px solid black;gap:2rem 2rem;font-size:1rem;color:black;}@media(max-width:992px){.cf-af21918d8656639646a9eb86a7ca06e2{box-sizing:border-box;margin:1.5rem;padding:1.5rem;background-color:black;width:75%;height:50%;border:3px solid black;gap:1.5rem 1.5rem;font-size:0.75rem;color:green;}}@media(max-width:576px){.cf-5a0d66d5b7e5c60dac49f3a86e4c1c8d{box-sizing:border-box;margin:1rem;padding:1rem;background-color:red;width:100%;height:100%;border:0px solid transparent;gap:1rem 1rem;font-size:1.5rem;color:orange;}}"`, |
There was a problem hiding this comment.
the fact that the hashes have changed here make me anxious. If this PR was a pure refactor, the hash would be the same
There was a problem hiding this comment.
Mmh, not sure what would be the correct prefix instead of refactor to be honest. Do you have a specific one in mind?
The hash generation changed as before we used the object of styles and stringified it before calling md5(). I changed it to use the resulting CSS string right away, so we have one less stringifying. The string itself is different (JS object vs. CSS rules) which is why the hash is different.
| expect(styles).toMatchInlineSnapshot( | ||
| `".cf-fb5147aff29cf3474d7f68cd382ba916{box-sizing:border-box;margin:0 Auto 0 Auto;padding:10px 10px 10px 10px;background-color:rgba(255, 255, 255, 0);width:100%;max-width:1192px;border:0px outside rgba(255, 255, 255, 0);gap:10px 10px;}.cf-1031d744e26de7b4e9422de0e74ae5cd{box-sizing:border-box;padding:0 0 0 0;background-color:rgba(255, 255, 255, 0);grid-column:span 6;border:0px outside rgba(255, 255, 255, 0);gap:0px 0px;align-items:start;justify-content:safe center;flex-direction:column;flex-wrap:nowrap;}.cf-4c0b9d7e44ec5898b6f5480ac5b1a634{box-sizing:border-box;padding:0 0 0 0;background-color:rgba(255, 255, 255, 0);grid-column:span 6;border:0px outside rgba(255, 255, 255, 0);gap:0px 0px;align-items:center;justify-content:safe center;flex-direction:column;flex-wrap:nowrap;}.cf-2745e04133568665b6d779d76342d59f{box-sizing:border-box;margin:0 0 0 0;padding:0 0 0 0;width:100%;max-width:none;}.cf-fe878a9e705110f2fee7b5698943918a{box-sizing:border-box;margin:0 0 0 0;padding:0 0 0 0;width:100%;max-width:none;font-size:16px;font-weight:400;line-height:20px;letter-spacing:0px;color:rgba(0, 0, 0, 1);text-align:left;text-transform:none;}.cf-0326e88429ff35544d5acf70391db2b2{box-sizing:border-box;margin:0 0 0 0;padding:0 0 0 0;}@media(max-width:992px){.cf-92b718a315702272accd18eb39a4a427{box-sizing:border-box;}}@media(max-width:576px){.cf-8187cd93307f9e0ceffff3cad17ce4d3{box-sizing:border-box;}}"`, |
There was a problem hiding this comment.
same question here - what made the hashes change?
packages/core/src/utils/styleUtils/ssrStylesStructureComponent.spec.ts
Outdated
Show resolved
Hide resolved
| 'font-size': '1rem', | ||
| }; | ||
|
|
||
| const res = createJoinedCSSRules(cssRules); |
There was a problem hiding this comment.
to me this function name sounds very crypric. What is the Joined CSS rule and what is the joined vs not joined css rule.
Maybe we call it toMinifiedCSS?
There was a problem hiding this comment.
I was thinking of the function Array.join which turns an array into a joined string. Happy to rename it though if that makes it easier to understand. As it does creat both normal CSS and minified CSS, I would like to go with stringifyCSSProperties (thinking of JSON.stringify) or something similar to that. Or is toCSS maybe enough?
There was a problem hiding this comment.
I left many comments, but I generally have these points as a reason why I don't think that this PR needs to be merged in this current state:
- The comments were left there on purpose, so that in a couple of months when we read that code, we can see a visual example of how the data gets transformed and understand what is happening without adding any extra logging. I am puzzled to understand why they are being deleted.
- I personally find certain namings too cryptic
- for example
buildCfStylesvsbuildCfStylesWIthSpecialCasing- I feel like we need to have only 1 function -buildCfStyles. The rest are additional transformers that we can call as a chain after callingbuildCfStyles, but now we have 2 functions that do the same, just 1 is smarter than the other and now I, as a developer, need to know when to call one and when the other (and that makes me question - why do we have 2 functions?) - I would rename
buildCfStylesto_buildCfStylesand then renamebuildCfStylesWIthSpecialCasingtobuildCfStylesand export that one publicly.
- Hash changes in the tests. The goal was to make sure that hashes stayed unchanged. The fact that they have changed in this PR, makes me alert, because that means that output styles are going to change to everyone who is using this SDK in prod with SSR. What made them change?
- The fact that instead of having hardcoded hashes, we are now testing their format. That check will never fail and that's not the purpose of those tests
| @@ -247,28 +147,6 @@ export const detachExperienceStyles = (experience: Experience): string | undefin | |||
| ); | |||
| }, | |||
| }); | |||
| /** | |||
| * propsByBreakpoint { | |||
There was a problem hiding this comment.
When I was reviewing the original PR, I found these comments quite helpful. Could we not remove them?
If we don't want them to bloat the code here, they could also be moved to the docstring of the respective functions that produce each data structure.
So this propsByBreakpoint example can be moved to the indexByBreakpoint function.
And the stylesForBreakpoint example later on can go into the doc string of builtCfStyles this way they are easy to see every time you use each function.
There was a problem hiding this comment.
I was having a hard time to visually parse the whole function. So moving it to the docstring of each function would be great. Will do that!
|
Hey @Spring3 @SofiaMargariti
|
Purpose
Improve the readability of critical code, reduce duplicated code, remove not needed code.
Approach
This is just a step towards the right direction. We're still running similar logic in multiple places without generalising it. I tried to reduce it slightly already but didn't mean to spend much more time on it atm.
TODO
ssrStyles.tsfor pattern nodes, cc @anwaar931