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

[WIP] Use Hooks #967

Merged
merged 34 commits into from Nov 5, 2019
Merged

[WIP] Use Hooks #967

merged 34 commits into from Nov 5, 2019

Conversation

emmatown
Copy link
Member

What:
Use hooks for stuff

Why:
There's a slight performance and size improvement(~150 bytes). I also have some ideas on how hooks could help some things so I want to do this so we can use hooks.

How:

Checklist:

  • Documentation
  • Tests
  • Code complete

Not gonna merge this until hooks are stable but just wanted to open this to show some people

@jgoux
Copy link

jgoux commented Oct 26, 2018

I made a codesandbox to demonstrate the usage of hooks with emotion 10 : https://codesandbox.io/s/3ykkxov026

Thanks for the advices @mitchellhamilton ! 🥇

type Props = { theme: Object }

// should we change this to be forwardRef/withCSSContext style so it doesn't merge with props?
// should we remove this altogether and tell people to useContext
Copy link
Member

Choose a reason for hiding this comment

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

we should! :) although we could simplify it for the users and provide

const useTheme = () => useContext(ThemeContext)

@Andarist
Copy link
Member

not sure what are ur plans regarding v10 timeline, but i agree with sentiment here reduxjs/react-redux#1065 (comment)

This was referenced Nov 8, 2018
@TrySound
Copy link
Contributor

TrySound commented Nov 25, 2018

Hey, guys!
I'm personally not ready to add jsx pragma into my project and would be cool to have a way around it. How about this kind of api?

import { useEmotion } from '@emotion/hook';

export const Component = () => {
  const { css, cx, theme, injectKeyframes, injectGlobal } = useEmotion()
  const container = css({ width: '100%', padding: theme.spaces[1] })
  const fade = injectKeyframes({
    from: { opacity: 0 },
    to: { opacity: 1 }
  })
  return (
    <div className={container}></div>
  )
}
  • it should work well with ssr
  • it's easy to add out of the box theming if necessary
  • per component caching is also possible
  • locally injected keyframes and globals can be destroyed along with component

I could work on it if you are interested.

@Andarist
Copy link
Member

Why the jsx pragma is a problem for you? Im curious - would be great to know people problems to potentially solve them.

As to the hooks - what would be the advantage of this over just importing "bare" emotion and using it in render?

@TrySound
Copy link
Contributor

Some tools (like fastpack) do not support jsx pragma. Flow had also problems with it, not sure about now.

The advantage and the ability to connect with react context to achieve caching and SSR. As far as I understand bare import is not just work in v10.

Also it would be really good to make injectKeyframes and injectGlobal destroyable.

@TrySound
Copy link
Contributor

jsx pragma looks to me like a workaround and it still adds a noise in dev tools like styled.

@netlify
Copy link

netlify bot commented May 22, 2019

Preview Docs

Built with commit 7e9b819

https://deploy-preview-967--emotion.netlify.com

# Conflicts:
#	.flowconfig
#	.flowconfig-ci
#	.github/ISSUE_TEMPLATE.md
#	package.json
#	packages/core/src/global.js
#	packages/core/src/jsx.js
#	packages/styled-base/src/index.js
#	yarn.lock
@changeset-bot
Copy link

changeset-bot bot commented Nov 5, 2019

🦋 Changeset is good to go

Latest commit: 6af574b

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Andarist Andarist changed the base branch from master to next November 5, 2019 10:41
}
cache.insert(``, serialized, sheet, false)
},
[cache, serialized]
Copy link
Member

Choose a reason for hiding this comment

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

previous version was only updating styles on serialized.name change, I'm not sure if the previous version was safe - because I don't quite understand the cursor/keyframes thing and what guarantees it gives in regards to serialized.name. Would love to learn about it though, could you explain this a little bit?

If we can't put serialized.name in the inputs array then we can remove this array altogether, as serialized changes with each render - so technically this input array doesn't give us anything right now

Copy link
Member Author

Choose a reason for hiding this comment

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

If serialized.name is the same, the style is the same since the hash of the contents of the keyframes is in the style it's part of the whole styles' hash.

@github-actions github-actions bot mentioned this pull request Nov 5, 2019
# Conflicts:
#	packages/core/package.json
#	packages/emotion-theming/package.json
#	packages/native/package.json
#	packages/primitives-core/package.json
#	packages/styled-base/package.json
#	packages/styled-base/src/index.js
#	packages/styled/package.json
@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 5, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6af574b:

Sandbox Source
Emotion Configuration

@codecov
Copy link

codecov bot commented Nov 5, 2019

Codecov Report

Merging #967 into next will increase coverage by 0.03%.
The diff coverage is 99.19%.

Impacted Files Coverage Δ
packages/emotion-theming/src/theme-provider.js 100% <100%> (ø) ⬆️
packages/primitives/src/test-props.js 100% <100%> (ø) ⬆️
packages/core/src/class-names.js 100% <100%> (ø) ⬆️
packages/primitives-core/src/styled.js 100% <100%> (+4%) ⬆️
packages/emotion-theming/src/with-theme.js 100% <100%> (ø) ⬆️
packages/styled-base/src/utils.js 100% <100%> (ø) ⬆️
packages/styled-base/src/index.js 100% <100%> (ø) ⬆️
packages/core/src/context.js 100% <100%> (ø) ⬆️
packages/core/src/jsx.js 100% <100%> (ø) ⬆️
packages/core/src/global.js 97.56% <97.14%> (-0.12%) ⬇️

@emmatown emmatown merged commit 7903605 into next Nov 5, 2019
@emmatown emmatown deleted the hooks branch November 5, 2019 22:32
@github-actions github-actions bot mentioned this pull request Nov 5, 2019
louisgv pushed a commit to louisgv/emotion that referenced this pull request Sep 6, 2020
* Tests are broken :( but it works

* Add displayName

* Use ref

* Remove stuff

* Update react-test-renderer

* Use useContext in more places

* Fix a Global insertion order bug

* Add a test

* Fix typo

* Remove passing the theme to the css prop, Global and ClassNames

* stuff

* stuff

* changes

* Add a test back

* Fix a thing

* Update stuff

* Update things

* Drop support for innerRef entirely

* Set correct peerDeps on react + upgrade react-related devDeps

* Fix tests

* Fix linting error

* Remove custom useContext flow types

* Fix flow error

* Fix layour effect input array to include serialized.name instead of whole serialized

* Add a changeset

* Add another changeset
@github-actions github-actions bot mentioned this pull request Nov 10, 2020
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

8 participants