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 setup function for pragma, forwardRef, and theme #76

Merged
merged 10 commits into from Feb 19, 2020

Conversation

@donysukardi
Copy link
Contributor

donysukardi commented Dec 17, 2019

Pending tasks

  • Tests
  • Documentation
  • Bundle size

This change increases the bundle size though. Any idea how we could trim it to fit the maxSize?

 FAIL  ./dist/goober.js: 1.03KB > maxSize 1KB (gzip)

 FAIL  ./dist/goober.module.js: 1.03KB > maxSize 1KB (gzip)

Related Issues: Theming (#61), forwardRef (#63)

return function Styled(props) {
const _props = (_ctx.p = Object.assign({}, props));
function Styled(props, ref) {
const theme = useTheme();

This comment has been minimized.

Copy link
@cristianbote

cristianbote Dec 17, 2019

Owner

Does it has to be a function? Could it be a plain object? Could become a perf bottleneck on user land.

This comment has been minimized.

Copy link
@donysukardi

donysukardi Dec 17, 2019

Author Contributor

This is to get feature parity with other CSS-in-JS theming capability, i.e.

const useTheme = () => useContext(ThemeContext)

Here's how it's similarly done in styled-components@5 - https://github.com/styled-components/styled-components/blob/v5/packages/styled-components/src/models/StyledComponent.js#L129

This comment has been minimized.

Copy link
@donysukardi

donysukardi Dec 17, 2019

Author Contributor

For non context user, it could be just () => theme

src/styled.js Outdated Show resolved Hide resolved
Copy link
Owner

cristianbote left a comment

This is so great! 🙏 Thank you for opening this!

@donysukardi donysukardi requested a review from cristianbote Dec 18, 2019
@cristianbote

This comment has been minimized.

Copy link
Owner

cristianbote commented Dec 18, 2019

This is really great! Really love it! How do you feel about it? Can you add the tests and everything? What can I help you with?

let h;
const setPragma = pragma => (h = pragma);
let h, forwardRef, useTheme;
const setup = (_h, _forwardRef, _useTheme = () => {}) => {

This comment has been minimized.

Copy link
@cristianbote

cristianbote Dec 18, 2019

Owner

How about not defining a default _useTheme? Would that break?

const _props = (_ctx.p = Object.assign({}, props));
function Styled(props, ref) {
const theme = useTheme();
const _props = (_ctx.p = Object.assign(theme ? { theme } : {}, props));

This comment has been minimized.

Copy link
@cristianbote

cristianbote Dec 18, 2019

Owner

Also what do you think?

Suggested change
const _props = (_ctx.p = Object.assign(theme ? { theme } : {}, props));
const _props = (_ctx.p = Object.assign(useTheme ? useTheme() : {}, props));

This comment has been minimized.

Copy link
@donysukardi

donysukardi Dec 19, 2019

Author Contributor

Technically it's possible, but usually useTheme just returns theme instead of { theme }, e.g. https://github.com/styled-components/styled-components/blob/v5/packages/styled-components/src/hooks/useTheme.js

It'd have to be like this instead,

const _props = (_ctx.p = Object.assign(useTheme ? { theme: useTheme() } : {}, props));

Let me give it a spin and see how the bundle size looks like, coz with gzip more code != bigger bundle size and vice-versa 😁

@cristianbote

This comment has been minimized.

Copy link
Owner

cristianbote commented Dec 18, 2019

Did I mention how exciting this looks? 😄 Can't wait to have the theme support in. I can see myself using it, for sure!

This is a good one! Really appreciate your time on this! 👍

@donysukardi

This comment has been minimized.

Copy link
Contributor Author

donysukardi commented Dec 19, 2019

Done the changes for both useTheme and ref. Bundle size seems to be lower now.

FAIL  ./dist/goober.js: 1.02KB > maxSize 1KB (gzip)

 FAIL  ./dist/goober.module.js: 1.01KB > maxSize 1KB (gzip)

I'll work on the tests for this.

@nksaraf

This comment has been minimized.

Copy link

nksaraf commented Dec 19, 2019

Hi,
I've been following the development of goober and absolutely love the simplicity of the code base. I have had to implement similar functionality for a fork. Is there anything I can do to help land this PR, or in other ways with the library.

@cristianbote

This comment has been minimized.

Copy link
Owner

cristianbote commented Dec 19, 2019

Hey @nksaraf thanks for checking goober all along! 🙂 Well anything that comes to mind! There a couple more things inside the issues that can be tackled as well. Give them a try see what you can do.

Thank you for your time!

@donysukardi

This comment has been minimized.

Copy link
Contributor Author

donysukardi commented Dec 26, 2019

@cristianbote I've updated the implementation slightly. I observed that in styled-components, the theme from context is not passed down to the component, i.e. it's just used to generate the className with necessary styles.

I've also added unit and integration tests for the changes.

@ryansolid

This comment has been minimized.

Copy link

ryansolid commented Feb 5, 2020

This was going strong up to a month or so ago. Is this PR still moving forward? Is something holding it up? Are we still holding out hope it can get 0.02 kb smaller?

@cristianbote

This comment has been minimized.

Copy link
Owner

cristianbote commented Feb 5, 2020

Hey @ryansolid! :) Thanks for stopping by!

Well, it's just really my lack of time to do a perf bench on this and check the sizes. Also, this will definitely be a major version upgrade, since the whole setPragma update.

@cristianbote

This comment has been minimized.

Copy link
Owner

cristianbote commented Feb 5, 2020

Oh and documentation about the changes.

@cristianbote

This comment has been minimized.

Copy link
Owner

cristianbote commented Feb 6, 2020

Had a look into this change sets and it seems that this would add some considerable size. Something like 1035B. Will dig more into it.

src/styled.js Outdated Show resolved Hide resolved
src/styled.js Outdated Show resolved Hide resolved
cristianbote and others added 3 commits Feb 18, 2020
Co-Authored-By: Jovi De Croock <decroockjovi@gmail.com>
Co-Authored-By: Jovi De Croock <decroockjovi@gmail.com>
@cristianbote

This comment has been minimized.

Copy link
Owner

cristianbote commented Feb 18, 2020

Hey @donysukardi things are happening! 🥳😎 Thanks to @JoviDeCroock for pushing this with his PR for autoprefixer. Overall this will end up under 1024B 🥳👍🥜 wohoo! So excited!

This will definitely be v2

@cristianbote

This comment has been minimized.

Copy link
Owner

cristianbote commented Feb 18, 2020

The tests failures are my doing 😑 sorry. I resolved the conflicts but somehow missed stuff. Will correct then asap

@donysukardi

This comment has been minimized.

Copy link
Contributor Author

donysukardi commented Feb 19, 2020

This is awesome! I am excited!! Let me know if there's anything else I can help with :D

@cristianbote

This comment has been minimized.

Copy link
Owner

cristianbote commented Feb 19, 2020

Awesome! @donysukardi I think @JoviDeCroock opened a PR against yours to fix the test failures. In here https://github.com/donysukardi/goober/pull/1/files I think that would make this to pass.

donysukardi and others added 2 commits Feb 19, 2020
@cristianbote

This comment has been minimized.

Copy link
Owner

cristianbote commented Feb 19, 2020

Alrighty! 😄 everything is fine. The above failures are expected 👍

Thank you soo much @donysukardi! 🎉 🌟

@cristianbote cristianbote merged commit cf0148d into cristianbote:master Feb 19, 2020
4 of 5 checks passed
4 of 5 checks passed
build
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
security/snyk - package.json (cristianbote) No manifest changes detected
security/snyk - packages/babel-plugin-transform-goober/package.json (cristianbote) No manifest changes detected
security/snyk - packages/gatsby-plugin-goober/package.json (cristianbote) No manifest changes detected
@donysukardi donysukardi deleted the donysukardi:setup branch Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.