emotion-js / emotion

👩‍🎤 CSS-in-JS library designed for high performance style composition
https://emotion.sh/
MIT License
17.43k stars 1.11k forks source link

Number in @emotion/native does not work #1543

Open imcvampire opened 4 years ago

imcvampire commented 4 years ago

Current behavior:

If I pass a number to styled, it doesn't work. I need to use toString()

styled.View`
  height:  ${{ paddingTop }: { paddingTop: number }) => props.paddingTop.toString()}px;
`

Expected behavior:

styled.View`
  height:  ${{ paddingTop }: { paddingTop: number }) => props.paddingTop}px;
`

Environment information:

https://snack.expo.io/H1xW-yR_S

Andarist commented 4 years ago

It shouldn't matter - because JS should automatically stringify this if you really are passing a number there.

Could you prepare repro case? Without such, we'll have to close the issue as not actionable.

imcvampire commented 4 years ago

Hi @Andarist, please kindly check https://snack.expo.io/H1xW-yR_S

Andarist commented 4 years ago

Ok, so I've debugged this a little bit and it seems that StyleSheet.create outputs an object with numbers as values and currently, we allow for interpolating those values (numbers) - so it's hard to differentiate between those special numbers and regular ones (like in your case).

The native library was developed by @nitin42 , maybe he could take a look at this or at least clarify if this is intended, how he believes it should work etc.

nitin42 commented 4 years ago

@Andarist thanks for looping in!

It's been a while since I last looked at the @emotion/primitives-core implementation so not sure about the issue exactly. Let me have a look at it this week and I'll post an update @imcvampire

nitin42 commented 4 years ago

Hey @imcvampire

An alternate solution for this will be to use interpolations in this manner:

const StyledText = styled.Text`
  ${props => props.size ? `font-size: ${props.size}px` : ''};
`

In this way, you can skip toString since the resultant value from the interpolation will be the combined string font-size: <your-prop-value>px. This should help.

Andarist commented 4 years ago

Both are workarounds though. Could you describe complexity of implementing a "proper" solution? I suspect it would require tracking more information about already parsed~ stuff to differentiate regular numbers from special StyleSheet IDs, right?

nitin42 commented 4 years ago

Yes, that's what I was thinking. I'll give it a go 👍

imcvampire commented 4 years ago

Hi @nitin42, do you have any update for this problem?

Thank you!

aecorredor commented 4 years ago

I know it's been a while, so just wondering if there's a workaround for this at the moment. Thanks!

Andarist commented 4 years ago

No workaround - somebody from the community has to step up to implement a fix for this as our team do not have resources to handle issues related to the React Native.

badsyntax commented 3 years ago

@Andarist do you have documentation or guidance on a fix? i'm more than happy to attempt a fix, but i don't understand some of the code, especially this line: https://github.com/emotion-js/emotion/blob/dcc72d06ace804330fd285a76c8574f3a89001f9/packages/primitives-core/src/css.js#L47-L48

This is the PR that introduced this feature, but there's no context on that particular line. I don't understand why isRnStyle = type === 'number'

@nitin42 have you given up providing a fix?

Andarist commented 3 years ago

This is the PR that introduced this feature, but there's no context on that particular line. I don't understand why isRnStyle = type === 'number'

In the past RN was "registering" styles within StyleSheet.create calls and the result of this function was an opaque number. This doesn't seem to be the case any longer. This has been changed in https://github.com/facebook/react-native/commit/a8e3c7f5780516eb0297830632862484ad032c10 and from what I see RN 0.56 was already released with this change.

badsyntax commented 3 years ago

Thanks @Andarist. I will send a PR. (I just need to figure out how to install the npm deps as the install is failing on my Apple M1 due to the sharp package. I will need to update some packages to allow me to install.)

Andarist commented 3 years ago

Oh, I have no experience with M1 problems (yet). So can't be of much help here - I would assume this is not a unique problem though so probably there should already be information somewhere on how to work around this.

badsyntax commented 3 years ago

@Andarist ultimately it comes down to using newer versions of the Gatsby plugins which in turn depend on newer versions of the sharp image library which provide support for arm64. I'd be happy to send a PR to fix things for Apple M1 users. A lot of the deps in this project are quite out of date. Is there any plan to update deps?

I still plan on sending a PR for this issue but been sidelined a bit. I'll get there!

badsyntax commented 3 years ago

I've looked into this some more. The fix is straightforward for react-native, but will break things for react-native-web, because RNW has not migrated StyleSheet to align with RN, and is still using number references.

Here's RNW: https://github.com/necolas/react-native-web/blob/13f7b9e2f45af2761bfdccd763c511992c0be030/packages/react-native-web/src/exports/StyleSheet/StyleSheet.js#L47-L57

Here's RN: https://github.com/facebook/react-native/blob/0.64-stable/Libraries/StyleSheet/StyleSheet.js#L360-L373

I've created an issue in the RNW repo: https://github.com/necolas/react-native-web/issues/2068

This is rather unfortunate.

I don't think there's much we can do until RNW aligns their StyleSheet implementation with RN.

See https://github.com/emotion-js/emotion/pull/2408 for some proof-of-concept changes - although the build fails in CI due to deps updates, the tests all pass locally.

pierpo commented 3 years ago

Isn't there any workaround for this? Can't we make it work at least for non react native web users?

Also, does it work on styled-components? If it does, how did they manage to make it work?

NordlingDev commented 2 years ago

@pierpo The workaround is to do numberValue.toString() I guess?

And to answer your question regarding if this works in styled-components. Yes, it does work.

pierpo commented 2 years ago

Yeah, but it diverges from web code where putting a number works directly. You're right, it does work on styled-components!

The problem with that is that it makes migrating from styled-components to emotion trickier 😉

This prettiest way I found to write it was this:

const Component = styled.View`
  margin: ${({ theme }) => theme.margin.xs + 'px'};
`
badsyntax commented 1 year ago

Related to https://github.com/emotion-js/emotion/issues/1543#issuecomment-867583957, I wonder if this issue can be revisited now that RNW has changed it's Stylesheet.create implementation to match RN, see release 0.18.0