Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AsyncStyleSheet and auto removing of unused styles #553

Closed
wants to merge 1 commit into from

Conversation

theKashey
Copy link
Contributor

I come from here #452
This PR not passing all tests(I dont know why), and linting (ok, this is just proof-of-concept)

What is done here:

  1. I replace StyleSheet with very simply AsyncStyleSheet. It just add styles in a bulk, and then compact them in a real huge style.
  2. I add removing of old styles. I was shocked, then I dont find anything about "remove" in Styled. You are very dirty ones. You should clean after you.

What in result

  • there is no need for dirty hacks on stylesheet level to support "millions of rules". You will never have millions of rules. Simultaneously.
  • you can animate your styles. With out penalty.
  • thanks to buck operations everything runs a bit faster
  • (cons) But in a bit different way.

@mxstbr-bot
Copy link

mxstbr-bot commented Mar 3, 2017

Fails
🚫 Please include a CHANGELOG entry. You can find it at CHANGELOG.md
Warnings
⚠️ There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

@geelen
Copy link
Member

geelen commented Mar 7, 2017

Ok I've had a bit of a read of this PR, and I understand it's just at a proof-of-concept stage, but I'm not convinced the problems described in #452 are in need of something like this. But I want to pick up some discussion from there and move it here, in particular:

The cons are simply - everything will be rendered in a strange way:

first everything will be rendered, and become invisible.
next styles will arrive to a browser, and whole tree will gain classNames. And regenerated.
then something changes style of component - they will not be changed instantly, but only after few ticks. Ie after flushing new styles to new stylesheet.

This seems very much the way Aphrodite works, and in particular the "big problem" of Aphrodite is the way the styles are injected after the markup has hit the page: Khan/aphrodite#76

However, the bigger dealbreaker for me is that ordering of CSS rules become unpredictable:

// Default rendering
<Comp/>

// After some user action
<Comp active/>

// Later 
<Comp/>

If you remove the css from Comp when active = false, then add it again later, you've reordered the CSS. Given we support descendant selectors, order is hugely important and informs a lot of the design of Style Components. It's issue #1 after all (worth reading through the linked issues there as well).

As a result, I'm not inclined to merge this or anything that adds a flash-of-unstyled-markup or involves reordering CSS, but I'm happy to continue the discussion, and I do appreciate the work you've put into this already, @theKashey.

@theKashey
Copy link
Contributor Author

@geelen - thanks for reply. I would like to discuss this PR.
So main goals is simply:

  1. Give ability to create new styles. quickly, instantly, for animations, from templates and so on.
    To achieve this you have to:
  2. Apply them in bulk.
  3. Remove unused styles. As long some styles during animation are using for a few moments.

Next I just propose the way, we use in Yandex for ages.

What about "problems":

  1. "Lag" before style is required and className set.
    Here I use timeout, just because dont have anything else. Originally it was "nextTick".
    In React LifeCycle you can create a TopLevel component with overriden ComponentDidUpdate(and so on) and flush styles on this event.
    Then all children "perform" style update, and you can apply all new styles in a bulk.
    In this case you have to return new className in a "old" way, synchronously. Everything will be same, but you will have "bulk".
    So - no problem here.

  2. I dont really understand issue.
    Why Horizontal and Vertical get rendered before SVG?
    Rendering must go from top to bottom. I should investigate this problem.
    But, from my point of view - it is very very bad idea, to rely on component creation order.
    Or, "desired" order can be emulated.
    Let me dive deeper inside this problem, I just always avoid situations like this.

@mxstbr
Copy link
Member

mxstbr commented Apr 1, 2017

But, from my point of view - it is very very bad idea, to rely on component creation order.

That's what styled-components has to do to work as people expect it to. 🤷‍♂️

@kitten
Copy link
Member

kitten commented Apr 19, 2017

The new stylesheet that @geelen wrote that is part of the async SSR effort, should deprecate this effort; Furthermore, I think this AsyncStyleSheet doesn't handle the injection order correctly, which leads to specificity and priority issues on the generated CSS

@kitten kitten closed this Apr 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants