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

Use StyleSheet.create in React Native #2060

Merged
merged 6 commits into from
Nov 11, 2020

Conversation

efoken
Copy link
Contributor

@efoken efoken commented Oct 30, 2020

What:

Change to StyleSheet.create for React Native

Why:

StyleSheet.create creates an instance for the given styles and we prevent re-creation of that instance when the same styles come in. Besides that StyleSheet.flatten which was used before, creates inline styles with React Native Web, while StyleSheet.create create atomic CSS classes.

How:

Change StyleSheet.flatten to StyleSheet.create with an extra check to prevent re-creation.

Checklist:

  • Documentation
  • Tests
  • Code complete
  • Changeset

@changeset-bot
Copy link

changeset-bot bot commented Oct 30, 2020

🦋 Changeset detected

Latest commit: 2404b39

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@emotion/native Major
@emotion/primitives Major
@emotion/primitives-core Major

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 30, 2020

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 2404b39:

Sandbox Source
Emotion Configuration

@efoken
Copy link
Contributor Author

efoken commented Oct 30, 2020

@Andarist any ideas how we can handle tests? Because StyleSheet.create simply returns a number and not the styles object.

@Andarist
Copy link
Member

Is there any way to retrieve the styles from some global registry or smth (when dealing with React Native)? I don't see anything fancy used in Styled Component's toHaveStyleRule which I was expecting to handle this.

Could you check if this sort of stuff works with SC? If yes then we could check how they do it.

If we won't be able to figure it out then we'd probably just merge this in and worry about that later - I would like for some research to happen first though so we could assess what to do based on the results.

@Andarist
Copy link
Member

Andarist commented Nov 7, 2020

@efoken friendly 🏓

would you be able to check the things mentioned in the comment above? I hope to release Emotion 11 in the upcoming week - it would be great if we could manage to merge this in before that.

@Andarist
Copy link
Member

Andarist commented Nov 7, 2020

I've done a little bit of digging and it seems that:

Because StyleSheet.create simply returns a number and not the styles object.

Is not actually true - this has been changed over 2 years ago: facebook/react-native@a8e3c7f#diff-28f312824659b615c3d5dba1bca670c630afbb7490258d6d820b90ef3d0f0027R372 . We might however deal with RNW or smth in tests here and it might have not adhered to that change.

Is there any way to retrieve the styles from some global registry or smth (when dealing with React Native)? I don't see anything fancy used in Styled Component's toHaveStyleRule which I was expecting to handle this.

This is where SC renders styles:
https://github.com/styled-components/styled-components/blob/28b4c28f1ebd1ea5a34717df2fe49634461285b1/packages/styled-components/src/models/StyledNativeComponent.js#L102
and all their Jest tests just use react-test-renderer. So this makes sense that there is no fancy logic in their matcher.

@efoken
Copy link
Contributor Author

efoken commented Nov 9, 2020

@Andarist I had a look on how styled-components is testing the styles, but what you must know is, that they are using the react-native Jest preset for the native tests with the style objects, which you can find here https://github.com/styled-components/styled-components/blob/28b4c28f1ebd1ea5a34717df2fe49634461285b1/packages/styled-components/src/native/test/native.test.js

So maybe we can setup another jest script using an extra config, what you think? The config styled-components is using: https://github.com/styled-components/styled-components/blob/28b4c28f1ebd1ea5a34717df2fe49634461285b1/scripts/jest/config.native.js

@efoken
Copy link
Contributor Author

efoken commented Nov 9, 2020

Because StyleSheet.create simply returns a number and not the styles object.

This actually is not true, it returns an object of numbers 😄 That why we use it like so:

const styleSheet = StyleSheet.create({ generated: styles })
return styleSheet.generated

@efoken efoken force-pushed the next-react-native-stylesheet-create branch from 3c46c7a to 4fdd827 Compare November 10, 2020 13:12
@efoken
Copy link
Contributor Author

efoken commented Nov 10, 2020

I'm working on fixing the tests for hours 😄 Because when I use Jest with react-native preset, some tests work fine. But when it comes to react-primitives, all native/primitives tests break...

import { StyleSheet } from 'react-native' <- works fine
import { StyleSheet } from 'react-primitives' <- mapped to 'react-native-web' and breaks

The reason why this breaks is, because 'react-native-web' creates some "object" with StyleSheet.create from which later the CSS class names are generated...

@efoken
Copy link
Contributor Author

efoken commented Nov 10, 2020

BTW, I also committed one change to flatten the styles before passing on to StyleSheet.create, this is necessary because the styles are always an array. I found this while I got some tests running with the 'react-native' Jest preset 🙂 And this also fixes some primitives tests, except for the ones in packages/primitives/test/css.test.js

@efoken
Copy link
Contributor Author

efoken commented Nov 10, 2020

@Andarist the issue definitely comes from 'react-native-web', because it always returns an object of numbers with StyleSheet.create ... I have no idea how we can get the real styles out there ...

@efoken
Copy link
Contributor Author

efoken commented Nov 11, 2020

@Andarist Sorry for spam 😄 Just fixed the primitive tests with wrapping the css function with a StyleSheet.flatten, which in React Native Web gets the styles from the returned number/ID: https://github.com/necolas/react-native-web/blob/6928fab9e0ef1e5bd0c10713b3b18b1b178d0bfb/packages/react-native-web/src/exports/StyleSheet/flattenStyle.js#L31

For the native tests I can set up a jest.native.js config and do a separate npm script for running them, do you think this would be a good solution?

@Andarist
Copy link
Member

@Andarist Sorry for spam 😄 Just fixed the primitive tests with wrapping the css function with a StyleSheet.flatten (which in React Native Web gets the styles from the returned number/ID)

That's an awesome fix! We probably should provide a custom matcher for this so people wouldn't have to write this by hand but this is not a blocker for this PR and can be done after v11 gets released.

For the native tests I can set up a jest.native.js config and do a separate npm script for running them, do you think this would be a good solution?

Could you describe shortly what's the current status of this? What's broken, why, and how the proposed solution might help here?

@Andarist
Copy link
Member

From what I see the same hack with StyleSheet.flatten works for those tests. Is there any concern about using it here as well?

@efoken
Copy link
Contributor Author

efoken commented Nov 11, 2020

StyleSheet.flatten of course works for the css tests, but not for the snapshot tests, because with the current test setup, the tests in 'native' also seem to run with React Native Web...

@Andarist
Copy link
Member

Ok, I see that this has only worked for packages/native/test/native-css.test.js but in packages/native/test/native-styled.test.js we can't call StyleSheet.flatten easily because we don't get access to that style prop directly as those are snapshot tests.

I'm fine with updating snapshots with those numbers for now. The important thing is that it's doable to retrieve styles even if they get registered as numbers. This seems to be fixable at matchers/serializers level and can be tackled in followup PRs.

I have pushed the snapshot update to your branch. Gonna look in a moment at fixing flow errors. Would you be so kind to create a changeset for this change?

@Andarist Andarist merged commit 139ea33 into emotion-js:next Nov 11, 2020
@github-actions github-actions bot mentioned this pull request Nov 11, 2020
@efoken
Copy link
Contributor Author

efoken commented Nov 11, 2020

... too late 😄

@Andarist
Copy link
Member

Hehe, ye - I've managed to get to it sooner in the end :P

Thank you for your work!

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.

2 participants