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

Add dynamic value support for extract static in the styled function. #279

Closed
wants to merge 14 commits into from

Conversation

tkh44
Copy link
Member

@tkh44 tkh44 commented Aug 22, 2017

What:
Add dynamic value support for styled (react components) when using extractStatic mode. This intentionally handicapped to just styled because of the inline style requirement.

Todos:

  • the styled component is one ugly hack and needs to be cleaned up.
  • css var support detection (use whatever postcss uses to determine if css var support is expected)
  • documentation on the limitations.

This will work

const H1 = styled.h1`
  font-size: ${p => p.fontSize};
`

Why:
It's possible.

Make @ai happy.

How:
This was actually a very straight forward process.

  1. Always create the static css
  2. replace any placeholders with the className and index ${name}-${hash}-${index}
  3. detect if there are vars but no content function in the higher order component
  4. add the inline style defs for the css vars to the elements props

Checklist:

  • Documentation
  • Tests
  • Code complete

@tkh44 tkh44 requested a review from emmatown August 22, 2017 04:31

// Help Needed:
// We should read the browser preferences for postcss
// and turn this on only if css vars are supported in their browser targets.
Copy link
Member

Choose a reason for hiding this comment

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

Copy link

@ai ai Aug 22, 2017

Choose a reason for hiding this comment

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

Detection:

browserslist(undefined, { path: processingFileName }).some(i => i.slice(0, 2) === 'ie')

It will detect IE and IE mobile.

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

I'm good with this overall but I'm not sure if we should use inline style, maybe we should do it how we did it pre-v7, it's pretty small. Also, using inline style wouldn't work in Preact right now, preactjs/preact#666

@tkh44
Copy link
Member Author

tkh44 commented Aug 24, 2017

@mitchellhamilton I have a PR in for preactjs/preact#666 here preactjs/preact#827


export function customProperties(baseClassName: string, vars: Array<inputVar>) {
const hash = hashArray([baseClassName, ...vars])
const varCls = `css-vars-${hash}`
Copy link
Member Author

Choose a reason for hiding this comment

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

I did this so that users could SSR and extract the custom property values, but I'm not sure if it will trip up in other places.

Copy link
Member

Choose a reason for hiding this comment

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

I have to write a test for it but I think we have to do something different to get SSR to work.

Copy link
Member

Choose a reason for hiding this comment

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

I was wrong! it works right now! 🎉

const objs = path.node.quasi.expressions.slice(0, composesCount)

if (objs.length) {
throw new Error('Cannot use composes in extractStatic mode.')
Copy link
Member

Choose a reason for hiding this comment

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

@tkh44 Do you think we should just go to inline mode instead of throwing an error. We also need to check if there are any interpolations in other places before going to extract.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that might be a good idea.

@codecov-io
Copy link

codecov-io commented Aug 25, 2017

Codecov Report

Merging #279 into master will increase coverage by 0.35%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
+ Coverage   90.37%   90.72%   +0.35%     
==========================================
  Files          21       22       +1     
  Lines         966     1003      +37     
  Branches      260      271      +11     
==========================================
+ Hits          873      910      +37     
  Misses         75       75              
  Partials       18       18
Impacted Files Coverage Δ
...ages/emotion/test/extract/extract.test.emotion.css 100% <ø> (ø) ⬆️
packages/babel-plugin-emotion/src/index.js 97.97% <100%> (+0.22%) ⬆️
packages/babel-plugin-emotion/src/parser.js 95.45% <100%> (+0.33%) ⬆️
packages/react-emotion/src/index.js 100% <100%> (ø) ⬆️
packages/emotion/src/index.js 94.75% <100%> (+0.36%) ⬆️
...kages/emotion/test/extract/server.test.emotion.css 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56097d0...5b3f20e. Read the comment docs.

@emmatown
Copy link
Member

Just a thought: extracted css calls won't work in random interpolations.

@tkh44 tkh44 closed this Sep 21, 2017
@Andarist Andarist deleted the extract-static-styled branch November 10, 2020 22:56
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

4 participants