Jarzka / stylefy

Clojure(Script) library for styling user interface components with ease.
MIT License
317 stars 10 forks source link

Text-shadow anomalies in Chrome #48

Open paintparty opened 4 years ago

paintparty commented 4 years ago

Not sure if this one is worth looking into ...

I noticed that with certain combinations of values passed to text-shadow, the selector generated by stylefy is empty, meaning no styling gets applied. I only see this in Chrome (I'm using Version 83.0.4103.61 (Official Build) (64-bit)). It works as expected in Firefox and Safari.

(use-style
  {:font-size "50px"
   :margin "50px"
   :color "red"
   ;:text-shadow "-2px 4px blue"
   :text-shadow "-3px 4px blue" ; This one fails for some reason
   ;:text-shadow "-4px 4px blue"
   })
paintparty commented 4 years ago

Since leaving the comment^ above. I noticed many more similar instances of non-deterministic buggy behavior in chrome(with many different css properties). I spun up a fresh project in Figwheel as a sanity check. Before adding any code that used stylefy/use-style, I noticed that there were a bunch of styles loaded into the _stylefy-styles_ element in the document head, all from a different project. So at this point i realized there was some caching happening automatically, and then found the section in the documentation that says caching with html5 local storage is on by default, and can be optionally turned off. Turning off caching seemed to solve the problems in Chrome. I believe the same problems would have been present in any browser had I been using it in the previous days for development.

You may want to consider making caching opt-in. Or if not, might be a good idea to make it more explicit (higher up in Readme) that caching is enabled by default and this could potentially make the local dev experience go haywire if the cache is not cleared or disabled.

One more thing that I noticed (not sure but may be related to problems I was having), is that use-style seems to get trapped in an infinite loop of scoped-selector creation and dom manipulation if and when a user makes a simple syntactical error like this:

(use-style
        {:font-size 50
         :margin 50
         :color :red
         :text-shadow [[-3 4 :blue]]
         :on-click #(js/console.log "hi")})

Obviously the :on-click handler^ should be be in a separate map, but this is a mistake I've made myself once or twice. If stylefy caching is enabled, and the user happens to have a call to (stylefy-cache/clear) in their code (because they were trying to clear the cache), this will happen:

2020-06-14 14 38 05

Jarzka commented 4 years ago

When HTML5 local storage caching was introduced for the first time, it was turned off by default. It was a new feature and I wanted to know how people would approach it. I noticed that very few developers knew about this feature or turned it on. I'm not a fan of switching default settings between versions (I have bad memories about this from PHP). However, since the caching can speed up the style generation a lot and should not cause problems, I turned it on by default.

I have used local storage caching with multiple projects running on localhost and haven't faced any issues. Even if styles from multiple projects are cached using the same local storage (same localhost domain), and loaded to every project, the style maps are all unique and are not tied to any project. However, if you are constantly working with multiple projects using stylefy, it's probably better to either

I updated the README file so that these facts are mentioned in the setup process.

Do you have an idea how to reproduce the caching issue you were having until you fixed it by turning caching off? It would be important to know if there is a hidden problem when using it in multiple projects in local environment.

Also, now when I think about this, I believe the problem here is not the fact that caching is turned on by default, but the fact that HTML5 local storage is shared between every app on the same domain. This also applies to every stylefy app developed on the same localhost domain (and port). There is no much benefit in it. It probably would have been a good idea to force every developer to use multi-instance support so that every app gets its own cache. However, I'm not sure if this is something we should change. Even if it's not difficult to start using stylefy's multi-instance support, it would be breaking change and thus require a major version change and a migration guide.

The second bug you mentioned is obviously something that should never happen, even if you have a syntax error in style map or clear the cache manually. I need to investigate it.

Jarzka commented 4 years ago

The second bug is caused by the fact that functions always have a different hash value:

(hash (fn [] (println "hello"))) ; 1776427376
(hash (fn [] (println "hello"))) ; 84346717
(hash (fn [] (println "hello"))) ; 2110943392

When a style, which contains a function, is used, it is hashed and converted to CSS. After it's added into the DOM, the component using the style is re-rendered. However, since the style produces a different hash every single time, it creates an infinite loop.

A simple solution for this would be checking the given style map and raising an error / warning if a Clojure function is found inside of it. I cannot think of an use case when we would want to use a function as value and convert it to CSS (Garden's CSSFunction is a different thing). And even if such use case is found, we would have to hash the function in some other way. Records are also a problem, but we could probably hash them "structurally".

paintparty commented 4 years ago

Thanks for your thoughtful response to the above issues!

I’m not quite sure how much insight I could provide as far as reproducing this particular caching issue. I was developing heavily, and exclusively, on on a single project with figwheel-main at localhost:9500 for about a week before i noticed it. Not sure, but I have a feeling it was related to the above mentioned typo of including :on-click fns in the stylemap.

I agree its probably not worth making any breaking changes as in theory it could be avoided if a dev is aware of and utilizes the existing multi-instance support.