Skip to content

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

Open
@lwhorton

Description

@lwhorton

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions