search

clj-commons / cljss

Clojure Style Sheets — CSS-in-JS for ClojureScript
https://roman01la.github.io/cljss/
Eclipse Public License 1.0
454 stars 27 forks source link

:cljss.core/media race condition / stale class name issue #50

Open lwhorton opened 5 years ago

lwhorton commented 5 years ago

This is a doozy.

We noticed that occasionally a [:print] media style declaration would render incorrectly. After setting up a minimal-case project, we were unable to recreate the issue at all. It seems that the size of a project (the number of defstyleds) plays an important role in recreating the bug. The issue happens inconsistently (not the same elements) and occasionally (not every time), which smells like a race condition.

The consequence of the problem is this: A [style block, generated class name] is occasionally the correct style block attached to a randomly incorrect class name. The incorrect class name will be any random class generated from another defstyled that does not have a gensym as part of the name (this was our first clue).

This is what happens on the first compilation:

guidebook.views.stress_test_results.subtitle_s = ...
[".guidebook_views_stress-test-results__subtitle-s{color:black;font-size:1.5625rem;margin:6.25rem 0 3.75rem;font-weight:300;text-align:center;}","@media print{.guidebook_views_priority-actions__h2-s{font-size:1.25rem;margin:3.75rem;}}"]

Notice that the .guidebook_views_priority-actions__h2-s is incorrect (it's not the expected guidebook_views_stress-test-results__subtitle-s).

If we save that particular guidebook.views.stress_test_results file and look at the compiled output again:

guidebook.views.stress_test_results.subtitle_s = ...
[".guidebook_views_stress-test-results__subtitle-s{color:black;font-size:1.5625rem;margin:6.25rem 0 3.75rem;font-weight:300;text-align:center;}","@media print{.guidebook_views_stress-test-results__subtitle-s{font-size:1.25rem;margin:3.75rem;}}"]

We get the correct class name under the @media print block.

We dug through the code to what we believe is the source of the problem: https://github.com/roman01la/cljss/blob/master/src/cljss/builder.clj#L10-L43 .

It looks like almost every place cls is used, it's passed in as a fn argument. In this particular builder there's a mix of using the provided cls and directly accessing the c/env*. The only thing I can imagine is that there's a race condition and/or a stale atom state causing a mismatch between the two values at the time of style generation.

We will start looking into a fix for this, but after a short glance it's not immediately clear the purpose of the c/env* atom, and why it's even needed? There's a reducer that modifies the atom's :id value https://github.com/roman01la/cljss/blob/master/src/cljss/collect.clj#L25 , but I don't understand why that couldn't be performed as a local var inside the fn instead of a global atom.

Any context you could provide would greatly help and ensure that we don't submit a PR that's completely bonkers.

prestancedesign commented 10 months ago

Hi,

To point out that we also encountered the same situation on our application. Have any investigations been carried out since then?