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

really try insertRule #157

Closed
wants to merge 11 commits into from
Closed

Conversation

mattkrick
Copy link

@mattkrick mattkrick commented Oct 10, 2016

Features:

I'm betting this is gonna be a hard sell, so here's the pitch:

  • Touching a style tag after creation is bad. Really bad. After changing it, the DOM must parse the text, then evaluate the rules. ALL the rules. This isn't cheap, and it grows in O(n) time as more styles get added. We could solve this by creating multiple tags, but having 4000 style tags is tough to debug. Plus, if you exceed that, IE10 yells at you. We can kick the can down the road by sticking fonts in their own tag, but then you get a flicker on initial load & for every font you async load. Also, if a tag gets large enough, it'll cause a flicker regardless of a font being in there or not.
  • This allows for styles to fail loudly. I think bugs like Support pseudo elements under pseudo styles #155 went uncaught largely because browsers parse style tags and toss out anything that doesn't make sense. The tossing stuff out is a silent fail. By using an API that doesn't fail silently, we can use it down the road when we create a dev/production build. Could you imagine? You could run QA without needing to know what the page is supposed to look like because it just logs an error if a style didn't load. I did that with my own app while writing this. It's a treat 👍
  • This opens the door to removing inline-styles-prefixer from the client build, saving ~3KB
  • This opens the door to removing asap since injections are so insanely cheap. This would solve Styles are injected after componentDidUpdate. #76 in the best way possible.

How it works:
The buffer used to store an big glob of text comprised of many rules. Now, the buffer stores an array of rules + for each rule a boolean called isDangerous. If a rule is dangerous, we use a try/catch block, which means V8 will bail on that function and never try to optimize it. That's fine because the calling function is externalized and the only things that trigger a dangerous flag are the vendor-proprietary things that they don't list in their computedStyles: ::-moz-placeholder, ::-moz-focus-inner or already prefixed globals that user is manually trying to inject. Otherwise, we inject with confidence.

We could whitelist these things, but by not using a whitelist, we save a little space in the payload. More importantly, we don't have to update the package whenever a browser vendor adds something. Also worth noting that the call is memoized so we only make 1 call to window.getComputedStyles and that array learns browser prefixes, so aphrodite gets (negligibly) faster the more you use it.

By avoiding prefixAll, we only generate rules that the client can understand. This is great because it is faster (less to generate = less to inject), gets rid of prefix problems (#100), and opens up the door to more helpful debugging.

Happy to answer questions. I know it's a big change, but for large apps, it's a necessary one. I know there are a few other folks like @abhiaiyer91 with large apps who have been struggling with the same performance bottlenecks, so I wanted to provide an option to stay competitive with JSS.

@xymostech
Copy link
Contributor

Hey @mattkrick!

Thanks for putting the effort into making these changes. Before trying to polish up the code, I have some broad feedback that I think will help us get this landed:

  • There are a lot of changes that you're making here, some which I think we want and some which will require some more thought. Putting them all into one pull request is going to make it hard to talk about the changes that are actually important here. In particular, I think a pull request that only adds the use of insertRule would provide much more useful discussion. Making changes to the font family keys or rewriting our stringify function at the same time is just going to make it harder for us to land.
  • The changes you made are failing a lot of tests (as you can see over at travis). Fixing up those tests and getting back to 100% code coverage will probably help us understand changes to the API better, and help us make sure these changes are reasonable.
  • It would be great if you followed our style guide (in particular, 4 space indents and 80 character line length limits)

So, if you could strip this down to the smallest diff that still demostrates what using insertRule looks like, and getting the tests to pass once you do that, that would be great!

@mattkrick
Copy link
Author

@xymostech oh wow, i didn't actually think this would get considered for master 😋. Yeah, it'll require a lot of rewriting of tests & jsdocs. Happy to do it if it's actually gonna get merged, but writing tests for rejected PRs is no good. Any feedback you can give me before then?

PS, any chance KA might adopt an .eslintrc or offer up an npm run lint command?

@xymostech
Copy link
Contributor

@mattkrick I just landed the pull request to add an eslintrc, npm run lint should work now if you rebase :)

@xymostech
Copy link
Contributor

@mattkrick As for advice, if this fixes the problems in #147 then I'm really interested in it. So, test to make sure it acutally solves that problem before putting in the effort?

@mattkrick
Copy link
Author

@xymostech it definitely solves it locally on firefox, chrome, and opera (we could even remove the 'o' prefix check, that hasn't been used by opera in...years?). Haven't tried IE since I don't have a windows box, but I'm pretty sure the getComputedStyles camelcase prefix is correct.
I'll update the staging server that you see in #147 so you can verify that the flicker is gone & then get some tests written up

@mattkrick
Copy link
Author

Alrighty, submitted 2 PRs that take care of the extra stuff. After those are merged, this PR will only have stuff relevant to #147. #155 can't be broken out since that bug prevents it from working.

@mattkrick
Copy link
Author

@xymostech what's your secret for getting 100% branch coverage? 🙈

@xymostech
Copy link
Contributor

Looks like you don't have any lines left to cover, but the command line output doesn't help with the other coverages. If you run

./node_modules/.bin/nyc --reporter=lcov npm run tests

it'll generate a much more usable HTML format for you to browse the coverage. In your case, the things I'm finding that aren't covered are:

generate.js:75
image

inject.js:109
image

inject.js:37 (you're always passing the "if" check here, and there's no else block. Maybe just get rid of this if)
image

util.js:202
image

util.js:246
image

It would be cool if this was auto-generated and hosted somewhere if the coverage fails...

@mattkrick
Copy link
Author

@xymostech ready for your 👀 .

I added the 4-space indent rule so i didn't have to manually fix up my stuff, it found some pre-existing stuff that wasn't compliant, hence the touches to webpack configs.

@mattkrick mattkrick mentioned this pull request Oct 13, 2016
@chendo
Copy link

chendo commented Oct 13, 2016

@mattkrick hopefully I have time to try this branch again today.

@jlfwong
Copy link
Collaborator

jlfwong commented Oct 13, 2016

We can kick the can down the road by sticking fonts in their own tag, but then you get a flicker on initial load & for every font you async load.

I'm a little confused by this. Why would there be a flicker on initial load? We could make one tag per font. Presumably you won't be using enough fonts to hit the IE10 limit.

Also, if a tag gets large enough, it'll cause a flicker regardless of a font being in there or not.

Is this an assumption or is there proof? I suspect the browser might lag a little, but it won't show you invalid results. I'm not certain either way though.

@mattkrick
Copy link
Author

@jlfwong The initial flicker would be caused by a switch from the fallback font to the downloaded font (assuming it doesn't get flushed immediately).If you can flush immediately, you shouldn't get a flash.

I'd also be very interested to hear from the folks who still see a flash after removing the fonts if this branch makes the flash go away entirely. TBH I thought a font that was inlined in base64 would be loaded in sync & wouldn't cause a flash, but it's still treated as a (cached) network request. You'd think the browser would create a checksum of the fetched file & compare it with the current...

const prefixedProperty = `${prefix}${capitalProp}`;
if (validProperties[prefixedProperty]) {
// learn which styles the browser likes
validProperties[validatedProp] = prefixedProperty;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validatedProp is falsy here (most likely undefined). Don't you want validProperties[property] == ... instead?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

digging in now, will have an answer in an hour or so...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's assume you have a rule like boxReflect.
It first checks to see if the browser can handle that by looking up boxReflect (Line 213).
If that doesn't work, then it tries all the prefixes. MozBoxReflect, webkitBoxReflect, ah, found it!
We know that the browser doesn't support boxReflect but it does support webkitBoxReflect.
So now we add that to the validatedProperties table: {boxReflect: 'webkitBoxReflect'}.
So the next time you look up boxReflect (Line 213) you grab the prefixed version immediately without having to set up the for loop.
The time savings is minimal, but aphrodite's target is IE10, so we can't use a Set, so we might as well make use of the object.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that. But following the control flow of the code, at this line (223), the value of validatedProp is falsy (an empty string (""), zero (0), false, undefined or null). Doing simple substitution, we arrive at either one of the following lines of code:

validProperties[undefined] = prefixedProperty;
validProperties[""] = prefixedProperty;
validProperties[0] = prefixedProperty;
validProperties[false] = prefixedProperty;

Is that really what you intended to do?

@abhiaiyer91
Copy link

@mattkrick testing this branch on our app now

@chendo
Copy link

chendo commented Oct 17, 2016

@mattkrick I've tried using the newest commit of this branch (https://github.com/mattkrick/aphrodite/tarball/d131a02923e6daa62df223e77b791eecaca9e067) and ran npm run build:main, but it appears to break compatibility with react-with-styles and react-with-styles-interface-aphrodite as StyleSheet doesn't appear to be defined anymore.

screenshot 2016-10-17 13 39 41
screenshot 2016-10-17 13 40 11

@mattkrick
Copy link
Author

@chendo i don't think you're referencing the branched version correctly. This PR doesn't do anything to the exports. For your convenience, try installing aphrodite-local-styles via npm and replace all references of 'aphrodite' to 'aphrodite-local-styles'. Note: I'll remove that package from npm if this gets merged.

@chendo
Copy link

chendo commented Oct 18, 2016

@mattkrick got it to work! Significantly decreased the amount of unstyled flicker we see, but we still see it occasionally in some scenarios. Thanks!

@mattkrick
Copy link
Author

@chendo in what scenario do you see a flicker still?

@abhiaiyer91
Copy link

@mattkrick EARLY Reads. aphrodite-local-styles work. Reduces flicker. Only reason flicker remains is I need to change more imports to local styles. I think we may go forward with this in the short term. our users are getting a terrible experience now

@mattkrick
Copy link
Author

@abhiaiyer91 you mean you have both aphrodite & my fork of it installed?

@yanivtal
Copy link

This is exciting! I guess it'll have to be rebased ontop of the latest extension changes. I noticed Aphrodite was just bumped to 1.0.0. Will that slow down merging this feature?

@xymostech
Copy link
Contributor

Sorry I haven't put time into reviewing this again, I've had other things on my plate. :( @yanivtal No, I just released 1.0.0 because we aren't really in an experimental state any more (we haven't been for a while actually, but better late than never?). If it's slow to get reviewed, it's just cause I'm slow!

@yanivtal
Copy link

Cool, no problem. Just making sure since it seems like this'll help a lot with perf. Just out of curiosity, is there any discussion about chunking across style tags for more perf? Seems we still have a big hit on every insert rule unless I'm missing something. @mattkrick says the multiple tags are harder to debug but I'm not familiar with that problem.

@yanivtal
Copy link

@mattkrick was this just waiting for Khan approval?

@mattkrick
Copy link
Author

Yup

@abhiaiyer91
Copy link

@mattkrick @xymostech Are we going to get this into master anytime soon?

@mattkrick
Copy link
Author

¯_(ツ)_/¯ At this point, I doubt it. FWIW, I use aphrodite-local-styles in production and it works great. Since I made this PR, Aphrodite now supports mixing global and local styles. I think that's dangerous, so I have no desire to keep my fork up to date with master when my version is more performant and disallows anti patterns. In the future, I'll shrink the client payload for mine and insert rules in sync so you don't have to use setTimeout hacks. If you'd like to take over getting this merged, feel free, but I'll close for now as it looks like my goals are no longer aligned with the project.

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.

Support inlined fonts Support pseudo elements under pseudo styles Styles flicker in.
7 participants