jhildenbiddle / css-vars-ponyfill

Client-side support for CSS custom properties (aka "CSS variables") in legacy and modern browsers
https://jhildenbiddle.github.io/css-vars-ponyfill
MIT License
1.46k stars 64 forks source link

[Feature Request] Observe on styles addition #4

Closed ilyazub closed 6 years ago

ilyazub commented 6 years ago

Do you have any plans for adding MutationObserver on style and link tags additions?

jhildenbiddle commented 6 years ago

Hi @ilyazub.

No plans currently, and to be honest I'm not sure this behavior should be baked into the ponyfill. I considered it early on but decided against it for the following reasons:

All that being said, there are valid counter arguments to made:

Hope that provides some insight as to why the ponyfill doesn't include this functionality.

jhildenbiddle commented 6 years ago

Hi again, @ilyazub .

Haven't heard back, so I'm going to close this issue.

Thanks again for the feedback. Much appreciated!

madeleineostoja commented 6 years ago

I know I'm late to the party on this one, but just wanted to chime in and say that I think having a mutation observer as an opt-in feature (as watch or observeUpdates or something) would be brilliant. Especially for SPAs, which is a super common use case for CSS vars.

IE 9-10 support is a non-issue IMO (less usage combined than IE8, and baseline these days is >IE11). Sure devs can fine-tune their own mutation observer impl, but that's premature optimization in most cases, where you can get all the low-hanging fruit with the existing exclude/include options.

jhildenbiddle commented 6 years ago

Thanks for the feedback, @seaneking. Happy to reopen the issues and discuss further.

I mulled this over yesterday. I understand the value and I am open to the change, but I still have some concerns.

Since developers understand and have control over when <link> and <style> nodes are added/removed/modified, my assumption was that it would be trivial to simply call the ponyfill next to the code that is making the change. This ensures that the ponyfill is only called when necessary, and avoids unnecessary or repeated calls that may occur when some other (potentially unknown) script also modifies the DOM and triggers the MutationObserver. The ponyfill can inspect mutations and determine if an update is really needed, but some amount of processing on every DOM change is unavoidable since attributes, childList, and subtree mutations all need to be monitored.

I guess my thinking is that while a MutationObserver could be handy, it may not be the best way to handle automating the ponyfill given the potential performance hit. My fear is that people will just flip the switch without realizing the "automatic" setting can slow down their app, potentially driving users away out of frustration, and potentially turning into lots of "your ponyfill is slowing down my app" issues being filed here on GitHub.

The counter argument, of course, is "it's opt-in, and you can just put all of the warnings in the documentation". Fair enough, I suppose. Perhaps a better understanding of the use case and just how common it is would be persuasive.

For clarification, is this the scenario you are hoping to address?

Thanks!

madeleineostoja commented 6 years ago

Yep can see the arguments both ways, up to you. Having a watch option is obviously never going to be as performant as manual calling, but it's not black or white either. For example with the state of automated bundling, chunking, and code splitting, I actually don't know what component styles are bundled where and exactly when they're parsed. Though by the same token updating on route change would be a safe bet for 99% of people.

My point is that fine tuning this stuff is probably premature optimization for a lot of people. In my experience one big global mutation observer is a negligible perf hit in the observing itself (perf becomes a problem when you're registering lots of concurrent observers), and as you say you can filter out ponyfill calls to only when needed. Then just add a note about the overhead vs doing it yourself.

ilyazub commented 6 years ago

If webpack allows to add a hook on CSS injection, MutationObserver in css-vars-ponyfill wouldn't be needed for me. That was the reason for my initial question.

Maybe it would be wiser to make a PR to webpack-contrib/mini-css-extract-plugin or something like this? Or to create a webpack plugin based on css-vars-ponyfill. Just thinking out loud...

@seaneking, @jhildenbiddle, what do you think?

Seems like loading of each <link> tag being resolved as a Promise: https://github.com/webpack-contrib/mini-css-extract-plugin/blob/98b4d0e69cf5563536f5be5bf0e4ae02dd73b107/src/index.js#L343

jhildenbiddle commented 6 years ago

I've decided to add this to the next release.

As I mentioned earlier, my initial pushback was based on the fact that the type of MutationObserver required would introduce some amount of processing on every DOM change. What I've come around to is that while this may be true, the real-world performance hit will likely be negligible if the ponyfill is smart about 1) checking to see if an update it needed and 2) only updating what is needed.

Quick overview of the proposed MutationObserver logic:

The one drawback is that if a <link> is injected, the external stylesheet will likely be requested twice: once by the browser and once by the ponyfill. If it's cached this isn't a big deal, but if not there will be a performance hit.

Thoughts?

madeleineostoja commented 6 years ago

:+1: Sounds great! More optimized than what most people would do if they called the ponyfill manually anyway

jhildenbiddle commented 6 years ago

Added in 1.8.0.

See options.watch and the changelog for details.

The optimizations included are likely more than most people would implement on their own (until they ran into issues), so that balances out the downsides of observing and checking so many mutations. If you wouldn't mind giving the new version a test run and letting me know how it works on your end, I'd very much appreciate it.

Thanks for the suggestion and discussion!