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

Try insertRule #83

Closed
wants to merge 1 commit into from
Closed

Try insertRule #83

wants to merge 1 commit into from

Conversation

pvolok
Copy link
Contributor

@pvolok pvolok commented Jun 6, 2016

This PR is a demonstration of using styleTag.sheet.insertRule instead of styleTag.appendChild (#76). The code is probably bad, I just made a minimum amount of changes to show the insertRule approach. It's not intended to be merge, just to discuss :)

This way styles are applied immediately in the css function. So in componentDidMount and componentDidUpdate all styles are already applied.

Also this approach scales better as the app continues to work, because browser doesn't have to reparse all css.

Problems:

  • insertRule('::-moz-focus-inner {}', 0) throws in chrome. If we wrap it with try-catch, we have about 25% performance penalty.
  • This API doesn't exists on IE8. Does aphrodite support it?

@@ -105,6 +107,7 @@ const stringHandlers = {
// This is a map from Aphrodite's generated class names to `true` (acting as a
// set of class names)
let alreadyInjected = {};
global.inj = alreadyInjected;
Copy link
Contributor

Choose a reason for hiding this comment

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

This wont work in the browser right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will, if built with webpack. But this shouldn't have been committed :)

@xymostech
Copy link
Contributor

I really like this! Might as well use browser APIs to make things better when we can!

useImportant) +
Object.keys(pseudoStyles).map(pseudoSelector => {
useImportant),
...Object.keys(pseudoStyles).map(pseudoSelector => {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this compile to something more complex than .concat?

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I feel like this code could be refactored a bit to make it easier to read. Each item in this array should probably be assigned to a variable with a name that describes its purpose and then those could be put into the array as needed...

@jlfwong
Copy link
Collaborator

jlfwong commented Jun 7, 2016

Thanks for the PR! Would you mind updating the description of the PR (and possibly the commit message) to make it clearer what the motivation for this is and how it accomplishes it?

Many of these questions are probably answered through #76, but as much as possible I like to avoid needing to jump around to answer the question "what does this do?". It also allows an eventual merge commit to be meaningful outside the context of GitHub (e.g. when reading through git logs in a terminal).

@pvolok
Copy link
Contributor Author

pvolok commented Jun 7, 2016

Added some explanation in the description.

@rainydio
Copy link

What is the reason to use prefixAll from 'inline-style-prefixer/static' instead of inserting vendor specific rules based on userAgent?

  • This should solve -moz- problems
  • Reduce CSS size
  • Unclutter developer tools

I understand that there is a server rendering case. But it might be more beneficial to build user-agent specific pages.

@rainydio
Copy link

I assume that's because browser detection takes up a lot of space. If inline-style-prefixer drops it, and replaces conditions with something smaller. They look quite small.

browser === 'firefox' && version < 15 ||
browser === 'chrome' && version < 25 ||
browser === 'safari' && version < 6.1 ||
browser === 'ios_saf' && version < 7

@kentcdodds
Copy link
Contributor

Those URLs you've specified are broken links.

@jlfwong
Copy link
Collaborator

jlfwong commented Jul 22, 2016

You're right that a benefit of using prefixAll makes the code size smaller, but it also facilitates better caching for server-side rendering. Otherwise, you have to make the user agent string part of the cache key. It's also just awkward in general for server-side rendering to do user agent detection because you'd have to figure out some way of passing along the user agent string in a way that doesn't break when multiple requests from different user agents are interleaved.

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.

5 participants