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

Consider toggling vendor prefixing on/off (stylis v3) #719

Closed
gajus opened this issue Apr 20, 2017 · 48 comments
Closed

Consider toggling vendor prefixing on/off (stylis v3) #719

gajus opened this issue Apr 20, 2017 · 48 comments

Comments

@gajus
Copy link

gajus commented Apr 20, 2017

This is regarding 2.0.0-15.

Using flex boxes results in:

screen shot 2017-04-20 at 13 36 48

There is absolutely no reason to add these... and it is likely a performance hit (processing unknown CSS properties is just another cycle).

There is a boolean way to check whether a property is supported in the browser. See Verou's article http://lea.verou.me/2009/02/check-if-a-css-property-is-supported/.

Possible performance issues aside, it is a nuisance from the DX perspective (as visible in the screenshot).

@k15a k15a added the question label Apr 20, 2017
@kitten
Copy link
Member

kitten commented Apr 20, 2017

@thysultan Is there any upside or downside to checking which CSS properties are supported in the runtime?

I don't think we had a discussion about this before, but please link it if I'm wrong 😄

@kitten
Copy link
Member

kitten commented Apr 20, 2017

So I've benchmarked this and it turns out it's fortunately not slow, but still, since it's O(n), we wouldn't want it in prod. Also the parsing of CSS outside of Stylis with a middleware would make this slower.

property in document.body.style x 4,233,144 ops/sec ±3.69% (53 runs sampled)

The big problem is: If we'd add this to development only, then we risk inconsistencies in prod.

@gajus
Copy link
Author

gajus commented Apr 20, 2017

The big problem is: If we'd add this to development only, then we risk inconsistencies with prod.

Whats the reason you would even think of adding this in dev only?

styled-components is runtime. It can check which styles to apply at the time of constructing the styles object.

@kitten
Copy link
Member

kitten commented Apr 20, 2017

Adding this to the runtime will make things slower. It's a question of how much slower, but at the same time we're trying to make v2 faster, so that's a conflict.

As mentioned, checking for a property runs with: 4,233,144 ops/sec ±3.69%

We'd want to cache this to get rid of unnecessary DOM access and speed things up. Without thinking about setting a property, getting one out of 10,000 runs with: 9,443,351.89 ± 0.74% op/s

This is most certainly not going to be faster, than the v8 optimised pattern matching and vendoring in stylis. Thus one argument is definitely performance.

The second part is that there's literally no reason to add this to production. The only argument is: It's a nuisance to some. So, that means in production this argument doesn't matter, since you wouldn't look and edit the styles in prod that often. It's really something you can live with there.

But then, if we'd only add this to development, if something goes wrong, or there's an inconsistency in an older / more unpopular browser (And, oh boy, there will be), then this either breaks in development, or it will produce inconsistencies.

@gajus
Copy link
Author

gajus commented Apr 20, 2017

Adding this to the runtime will make things slower. It's a question of how much slower, but at the same time we're trying to make v2 faster, so that's a conflict.

As mentioned, checking for a property runs with: 4,233,144 ops/sec ±3.69%

You are measuring styled-components performance, not the overall performance of the operation. Thats the pitfall in benchmarks. You are ignoring that the browser will still need to process the unknown properties, possibly at the larger cost.

Furthermore, I don't know how you are benchmarking this. Have you cached the check of a specific style property existence?

This is most certainly not going to be faster, than the v8 optimised pattern matching and vendoring in stylis. Thus one argument is definitely performance.

What "v8 optimized pattern matching"?

The second part is that there's literally no reason to add this to production. The only argument is: It's a nuisance to some. So, that means in production this argument doesn't matter, since you wouldn't look and edit the styles in prod that often. It's really something you can live with there.

The main argument (as you said yourself) is consistency between development and production environments.

But then, if we'd only add this to development, if something goes wrong, or there's an inconsistency in an older / more unpopular browser (And, oh boy, there will be), then this either breaks in development, or it will produce inconsistencies.

Right.

@gajus
Copy link
Author

gajus commented Apr 20, 2017

You are measuring styled-components performance, not the overall performance of the operation. Thats the pitfall in benchmarks. You are ignoring that the browser will still need to process the unknown properties, possibly at the larger cost.

Not to mention that the problem would not exist at all if the parser did not add these vendor prefixes in the first place without checking if the environment requires it.

@kitten
Copy link
Member

kitten commented Apr 20, 2017

  • This is a micro benchmark just for the check, as you can assume from the high ops/s number
  • Two words: (v8 optimized) (pattern matching) in stylis

Yeah, I'm sure @thysultan and @mxstbr have more to add about this decision.

If we'd do this, it'd be an extra check + cache on top of what stylis is already doing, I assume. What they'd remove is a simple check of whether a property has to be vendored, which I assume is faster than a DOM API / memory access from a cache-object. I'm only laying out some arguments here, maybe you could do the same, because I do not believe that this is really sth that will resonate with people:

DX is shit with vendor prefixes. I cannot edit CSS in web dev editor because presence of the prefixes requires me to untick every single instance of the property to affect the change

As for this:

and it is likely a performance hit (processing unknown CSS properties is just another cycle)

I'm pretty sure we can assume, that the browser parsing some CSS properties is not a bottleneck.

So at the end of the day I'd love to have more input from multiple people, and less passive aggressiveness, because I really want more of that in my life, right 😉

@thysultan
Copy link

Yes, the browsers css parser will probably never be the bottleneck, it's working on a much lower level than any javascript you can write, apart from that doing the check adds an unknown complexity of consistency and correctness since SSR is supported.

@bengotow
Copy link

Hey folks - just wanted to chime in regarding DX. I'm building an application that targets Electron (essentially I'm only building for the /very/ latest version of Chromium), and I'd really like if I could turn off all vendor prefixing. I don't need it and it makes things look messy in the inspector, and harder to change at runtime (because removing a property causes Chrome to fall back to it's second choice prefixed property.)

@geelen
Copy link
Member

geelen commented Apr 22, 2017 via email

@gajus
Copy link
Author

gajus commented Apr 22, 2017

Yeah I could see an argument for using a prefixless version of stylis, but
Phil is 100% correct the complexity of runtime CSS feature detection is far
greater than the benefits.

Not sure what code you are referring to.

A check of whether a property is supported by a browser is literally a single condition ('opacity' in document.body.style). A build time complexity of browser vendors is a different story, but thats irrelevant styled-components.

@geddski
Copy link

geddski commented Apr 25, 2017

IMO the ideal would be to let devs turn off the prefixing if they want during development. Otherwise tweaking values in Chrome devtools is a total pain.

@geddski
Copy link

geddski commented Apr 25, 2017

something like styled.disablePrefixes(!isProd)

@geelen
Copy link
Member

geelen commented Apr 25, 2017

@gajus it's not irrelevant, generating exactly the same output on the server and client is super important. Not least for rehydration purposes.

As I already said, I do like the idea of allowing all prefixes to be turned off in the case of Electron or, just for convenience during dev. Not a fan of @geddski's API suggestion—we've had a long-running battle in SC about how we could expose any configuration like this, and have so far settled on using different entry points styled-components/no-parser, for instance. But I think we need a better solution tbh, since having to change every import kinda sucks.

So yeah, no-prefix mode is on the roadmap, not sure when I'll get to it, but you'll just have to live with them for the moment.

@geddski
Copy link

geddski commented Apr 25, 2017

@geelen glad to hear it! Yeah imports isn't ideal because you'd either have to change them every time you ship or have dynamic imports which could be tricky.

I've seen a lot of libs have users add an entry in their webpack config file, seems like it could be a good place for it.

@SanichKotikov
Copy link

Hi. On/off prefixing is a good thing, but what about target browsers setting?

@kitten
Copy link
Member

kitten commented Apr 28, 2017

@SanichKotikov stylis doesn't support changing the targeted browsers, and it just prefixes everything down to IE8+ etc

@matt-erhart
Copy link

Does HMR solve this DX problem?

@k15a
Copy link
Member

k15a commented May 2, 2017

@matt-erhart why should it solve the DX problem?

@matt-erhart
Copy link

One of the main DX complaints is they can't do live editing of css in chrome. Doesn't HMR accomplish the same thing, i.e. instant feedback of changes like that?

@geddski
Copy link

geddski commented May 2, 2017

HMR is a pipedream

@Riokai
Copy link

Riokai commented May 11, 2017

I'm working on electron app and don't need vendor prefixed whatever development and production.

@thysultan
Copy link

This will land in V3 of stylis that s-c could potentially use, though it is only a on/off option.

@gribnoysup
Copy link
Contributor

I want to put my two cents in this discussion:

I agree that prefixes are messing with the DX, if you want a quick way to tweak styles in DevTools it is a pain to enable/disable some prefixed styles. HMR solves this problem, but recompile process could be quite slow on big projects, so it is a solution, but not ideal.

If stylis would support disabling prefixes it would be very nice to have this as an opt-in option in styled-components.

Overall styled-components are already ~= great DX, so our team would still happily use it without this fix for prefixes 😸

@kitten kitten added this to the v3.0 milestone May 22, 2017
@kof
Copy link

kof commented Jun 21, 2017

Any runtime solution costs extra size and performance for users.

Thats what I thought before I started experimenting. It turns out it is smaller than a static prefixer, highly performant for 2 reasons:

  1. Test results can be cached and tests are cheap (basically prop in object)
  2. You only add one version of a property or value, not all possible for all supported browsers, Which is a huge perf. bottle neck especially doing frequent updates.

The biggest challenge was the consistency with server-side rendered prefixes, which is hopefully soon getting fixed.

@oleksiibilous
Copy link

I love your idea of adding only browser spesific prefixes and caching. It will reduce props size comparing to the current solution and caching will minimize extra complexity as much as possible. Definitely +1 from me

But there is one thing. As I said any css in js solution is a transpiler and it should be configurable. Dynamic autoprefixer together with pre configured static autoprefixing could give you maximum performance win (e.g. you won't need to decide, which prefix to use if app supports only webkit browsers)

The more I think about this issue the more I understand that css-in-js solutions require their own JSX alternative or at least babel plugin, which will cover such things as static autoprefixing (in addition to the dynamic, which you've proposed), minification and helper functions or syntax sugar on build phase

I hope that will happen one day. But for now implementation of your solution inside styled-components can bring huge performance optimization

@thysultan
Copy link

In respect to stylis, i see this as more of a DX problem than a performance one since the stylis prefixer is already very fast, which might mean that adding additional config to tailor for specific/exact vendor prefixes might regress its perf(though largely marginal) than improve it when it comes to runtime perf.

@kof
Copy link

kof commented Jun 21, 2017

@thysultan stylis code is hard to read, how do you detect which prefix to add, whats the logic?

@oleksiibilous
Copy link

oleksiibilous commented Jun 21, 2017

Hi @thysultan

I do not favor adding any extra logic to runtime. Moreover, I wonder why would you add into runtime things which can be covered on the build stage

is already very fast

It doesn't mean that it couldn't be even faster and more lightweight, right?

But basically, I've just wanted to ask if there are any plans relating to the creation of transpiler, which will remove from runtime things that shouldn't be there and will bring great performance boost

P.S. Regarding benchmarks. I think they are not valid since you are comparing the thing, which works in runtime, to preprocessors, which compile code on a build stage. It will be great to see the comparison between stylis and preprocessed css

@oleksiibilous
Copy link

@kof as I understood it simply adds prefixes based on property key. Checkout this

@kof
Copy link

kof commented Jun 21, 2017

Compares what with what? Can you give a simple example that illustrates the logic? How do you decide if property needs a prefix?

@thysultan
Copy link

thysultan commented Jun 21, 2017

For example to prefix flex: something

We have the first three character codes f: 102, l: 108, e: 101 we multiply each of these codes by the index we expected them to be in +1, so now we have hash = (102*2+108*3+101*4). This is where the number 932 comes from, since we know before hand what hash the first three characters fle should produce, if it matches we concat vendor prefixes, if it doesn't nothing happens.

Edit: This is only for property vendor prefixing, prefixing something like a ::placeholder selector is done differently.

@kitten
Copy link
Member

kitten commented Jun 21, 2017

@kof thanks for linking to that prefixer. Maybe we can come up with a combination with the planned standard.

For everyone, generally discussing this right now doesn't make a lot of sense since we're working on a universal preprocessing pipeline that'll work both, during a build step, or in the runtime.

One thing that's important is that SSR works correctly. But anyway, once we have this stuff in place it should be easy to discuss customisations for the css pipeline

This means that how we're currently dealing with parsing css will change dramatically.

@oleksiibilous
Copy link

@philpl great to hear! Universal pipeline sounds promising. In such case, I will proceed with the styled-components in production. It gives me a huge boost in DX now and should improve performance a lot in the nearest future. Thanks for your work and get luck😉

@geelen
Copy link
Member

geelen commented Jun 22, 2017

Just raised this to continue the discussion re: universal pipeline: cssinjs/istf-spec#24

@callumlocke
Copy link

callumlocke commented Aug 7, 2017

Just to point out, this problem is not just a matter of aesthetics. Unwanted prefixes are a footgun: if you click to disable a property in the devtools, the browser may quietly fall back to some prefixed property. If you fail to notice this, it can lead to minutes or hours of pointless layout debugging. And even when you do notice it, it's painstaking to make sure you catch each potential fallback property and disable them all. It's effectively not even worth using this feature of the devtools, as it's easier to make the change in your code and wait for it to rebuild and then change it back again.

@mxstbr mxstbr removed this from the v3.0 milestone Dec 18, 2017
@ivanwolf
Copy link

ivanwolf commented Dec 7, 2018

I need to generate a css file and I'd love not having the prefixes. I'm building a PDF generator using python library weasyprint who doesn't care about prefixes.

@Carduelis Carduelis mentioned this issue Dec 18, 2018
14 tasks
aretecode added a commit to aretecode/modern-stack-web-portfolio that referenced this issue Apr 11, 2019
aretecode added a commit to aretecode/styled-components that referenced this issue Apr 20, 2019
- styled-components#719

- this is implemented using `process.env`, though we could implement it many other ways, since stylis is instantiated immediately, this is the most straight forward & minimal way to achieve the feature, and will have no performance impact if a bundler defines the env & dead code elimination takes place

- a test suite for stringifyRules is created
- two test cases are added, both of them use jest.resetModules() to ensure the stringifyRules file is evaluated with the updated process.env, only for this test case
1. to check the default functionality (having prefixes)
2. with the `STYLIS_SHOULD_PREFIX` env set, expect that there are no prefixes
- teardown/afterAll ensures the process.env is reset
@jurosh
Copy link

jurosh commented May 16, 2019

Joining party. Initially thought it was webpack causing all those prefixes - but ended up here, and reason why I see prefixes are actually styled componnets.

Adding my points. For development:

  • Incredibly hard to debug css in browser inspector, cause another prefixed rule might be still holding that property after turning off
  • Incredibly hard to see current css propertied because of mess of strikedthrough prefixed attributed

For production:

  • For modern browsers, there is need for very few of those prefixes
  • Lot of prefixes are for MS Explorer - who the hack still supporting that browser in 2019 ?

Would be really helpful turning prefixes off - or setting to add only most important and not prehistoric ones, as at least most of our users, are already using recent desktop browsers - and if not we are forcing them by popup to update.

@karlhorky
Copy link
Contributor

karlhorky commented Sep 22, 2019

Just to point out, this problem is not just a matter of aesthetics. Unwanted prefixes are a footgun: if you click to disable a property in the devtools, the browser may quietly fall back to some prefixed property.

+1 to this.

Not only that, but this can also lead to bugs with things that are incorrectly implemented / missing in Stylis, such as background-clip:

#2237

thysultan/stylis#132

@zeitamin
Copy link

you can now use disableVendorPrefixes prop on StyleSheetManager in v5rc3. I just tested it, it's much better without prefixes :D

@karlhorky
Copy link
Contributor

Looks like Stylis v4 will have a prefix for background-clip so at least this will work when it is released and styled-components is updated:

thysultan/stylis#132 (comment)

@czzonet
Copy link

czzonet commented Oct 21, 2021

you can now use disableVendorPrefixes prop on StyleSheetManager in v5rc3. I just tested it, it's much better without prefixes :D

https://styled-components.com/docs/api

@fabb
Copy link

fabb commented Oct 21, 2021

Aren’t there some that could still be relevant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests