emotion icon indicating copy to clipboard operation
emotion copied to clipboard

Number in @emotion/native does not work

Open imcvampire opened this issue 4 years ago • 19 comments

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:

  • react version: 16.9.0
  • react-native version: 0.61.2
  • emotion version: 10.0.20

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

imcvampire avatar Oct 11 '19 08:10 imcvampire

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.

Andarist avatar Oct 11 '19 10:10 Andarist

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

imcvampire avatar Oct 11 '19 11:10 imcvampire

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.

Andarist avatar Oct 11 '19 20:10 Andarist

@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 avatar Oct 14 '19 07:10 nitin42

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.

nitin42 avatar Oct 15 '19 21:10 nitin42

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?

Andarist avatar Oct 15 '19 21:10 Andarist

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

nitin42 avatar Oct 16 '19 08:10 nitin42

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

Thank you!

imcvampire avatar Oct 24 '19 02:10 imcvampire

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

aecorredor avatar Aug 22 '20 04:08 aecorredor

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.

Andarist avatar Aug 22 '20 05:08 Andarist

@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?

badsyntax avatar Jun 15 '21 14:06 badsyntax

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.

Andarist avatar Jun 15 '21 20:06 Andarist

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.)

badsyntax avatar Jun 16 '21 06:06 badsyntax

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.

Andarist avatar Jun 16 '21 21:06 Andarist

@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 avatar Jun 17 '21 12:06 badsyntax

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.

badsyntax avatar Jun 24 '21 12:06 badsyntax

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?

pierpo avatar Sep 03 '21 18:09 pierpo

@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.

NordlingDev avatar Feb 19 '22 21:02 NordlingDev

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'};
`

pierpo avatar Feb 23 '22 12:02 pierpo

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

badsyntax avatar Nov 28 '22 12:11 badsyntax