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

Compatibility with react-native-web 0.11 #2453

Merged
merged 2 commits into from May 11, 2019

Conversation

Projects
None yet
7 participants
@MrLoh
Copy link
Contributor

commented Mar 19, 2019

Using styled-components with react-native-web broke after upgrading to react-native-web 0.11, as they removed the legacy default export, see https://github.com/necolas/react-native-web/releases. This provides a fix by using import * as which seems more idiomatic anyways.

@probablyup

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

These things are not exactly equivalent...

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

@probablyup True, but does anything speak against doing it this way? Maybe it’s better to explicitly import the dependencies.

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@probablyup Any comments, compatibility with RN web might not be the highest thing on styled-components priority list, but it would be really nice to maintain, this is a valid use case, I would say.

@probablyup

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

The tests aren't passing yet... my guess is you'll need to write some code to recreate the import such that it'll work for both 0.10 and 0.11

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Finally found time to have a look at this. Seems a bit tricky to find a solution, since extra care is taken here to lazy-load react-native modules. Using import * as breaks that and the test fails because certain react native components have been deprecated and are not lazy-loaded anymore.

The only thing I could think of would be to use import() syntax in ES6 to load these modules upon request instead of react-natives workaround.

@PeterKottas

This comment has been minimized.

Copy link

commented May 9, 2019

Would it maybe make sense to just add

import { StyleSheet } from 'react-native';

And then on line 17 change

const InlineStyle = _InlineStyle(reactNative.StyleSheet);

to

const InlineStyle = _InlineStyle(StyleSheet);

That way it will work as long as you don't use the aliases.

The aliases might be fixed in a followup PR and at least this would allow some way of using RNW

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@PeterKottas Oh I didn’t even realizes that it is completely impossible to use together at the moment even with a styled(View) Syntax. Do you want to create a separate PR for that.

I don’t know how to possibly get styled.View back to work, since I don’t know enough about what this all gets compiled to and how to lazily import it the right way, since my knowledge of web pack is way too limited.

@PeterKottas

This comment has been minimized.

Copy link

commented May 10, 2019

Yeah definitely broken because of that line 17. Don't have the code pulled down so it would be quite an overhead for me to do the PR at the moment. I am happy to stick with older RNW for the time being, just wanted to share an idea :)

When it comes to the aliases, all you need to do on line 38 is replacing "reactNative[alias]" with something along the lines of "(await import(react-native'))[alias];"

Obviously, the await usage would require the function to be async. Hope that helps.

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Ok, I updated this PR, maybe we can at least merge this minimal change @probablyup. I don't see how async import() could possibly work here since this has to happen synchronously.

Maybe RNW is correct and the way styled.Component works is just incompatible with lazy loading and styled(Component) is what a performant syntax for ReactNative has to look like.

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

The only other Idea I have hear would be to normally import the core ReactNative components that one would use in RNW (View, Text, ScrollView, Image, FlatList, SectionList, TextInput, TouchableHighlight, TouchableOpacity) and enabling the styled.View syntax for those and the styled(Component) syntax for everything else, but it might be better to just overwrite the library like that in our own projects, since things will just break in unexpected places otherwise.

@PeterKottas

This comment has been minimized.

Copy link

commented May 11, 2019

Nice all checks passing! Well, maybe you just can't do these aliases dynamically. Tbh, I don't like the way it's done anyways. This convenience api should be optional at best and definitely not included in the main index file. Having it here IMHO forces the bundler to include everything at all times. I understand that's not such a big deal for RN, but RNW is a whole different story. What we are trying to do here will, in fact, beats the purpose of removing the default export. The whole point of that is to decrease the bundle size. I am considering maybe creating my own index file locally in the project by copying most of this and just removing the aliases.

@probablyup
Copy link
Contributor

left a comment

LGTM

@probablyup probablyup merged commit 75aa1b6 into styled-components:master May 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PeterKottas

This comment has been minimized.

Copy link

commented May 11, 2019

Awesome, any idea when will this be released @probablyup?

@probablyup

This comment has been minimized.

Copy link
Contributor

commented May 11, 2019

@PeterKottas

This comment has been minimized.

Copy link

commented May 11, 2019

Sounds good. We're doing a release in a few days and I'd be thrilled if this can be in it. Cheers.

@PeterKottas

This comment has been minimized.

Copy link

commented May 13, 2019

Hi @probablyup. Just a quick bump about this. That patch would be more than awesome for us ;) Cheers

@probablyup probablyup referenced this pull request May 30, 2019

Merged

v4.2.1 #2574

@z0al

This comment has been minimized.

Copy link

commented May 31, 2019

This doesn't work (even after v4.2.1).

Here how you currently import RN:

import reactNative, { StyleSheet } from 'react-native';

But you're still importing something that doesn't exist reactNative and it's actually used here:

return styled(reactNative[alias]);

So we still get the same error:

../node_modules/styled-components/native/dist/styled-components.native.esm.js
Attempted import error: 'react-native' does not contain a default export (imported as 'reactNative').
@PeterKottas

This comment has been minimized.

Copy link

commented May 31, 2019

Yeah sure, that's "intentional". The aliases are broken but you can at least use it without aliases. In other words, you can't do

styled.Text ...

but you can do

styled(Text) ...

by importing Text yourself.

Tbh, I don't see a point in having aliases for RN. It makes sense for the web where you don't have to import html tags, but RN ... It only makes it less maintainable from libs perspective IMHO.

If you need to update your code to omit aliases, you can easily do that by using regex in vs code with search and replace. Basically, you just modify styled.ANYTHING to styled(ANYTHING) and then search for missing imports and autoadd them. I did this for a huge solution that contains hundreds of pages and components and it took maybe 20 minutes.

@z0al

This comment has been minimized.

Copy link

commented May 31, 2019

I don't think that was "intentional" since it's a patch release. This is breaking change!

Also, I don't see the problem with the first approach (using the * syntax), like this:

import * as reactNative from 'react-native';

And yes, the tests were failing due to:

Invariant Violation: Navigator is deprecated and has been removed from this package. It can now be installed and imported from `react-native-deprecated-custom-components` instead of `react-native`. Learn about alternative navigation solutions at http://facebook.github.io/react-native/docs/navigation.html

cc @probablyup

@PeterKottas

This comment has been minimized.

Copy link

commented May 31, 2019

Breaking change is a change that breaks something that was previously working or a change that results in API changes. This is neither. This is merely a partial fix. Before this release, styles-components didn't work with RN at all. Now you can at least use it without aliases. Therefore no breaking change.

On the contrary, this

import * as reactNative from 'react-native';

is bad. As it's forcing bundler to include all modules even if you don't need/use them. That's why it was removed from RNW.

@PeterKottas

This comment has been minimized.

Copy link

commented May 31, 2019

To put it in other words, my recommendation to remove aliases is simply a recommendation. There was no decision taken to remove these. They just don't work at the moment. My suggestion would allow you to start using this before this remaining problem is fixed.

@z0al

This comment has been minimized.

Copy link

commented May 31, 2019

As it's forcing bundler to include all modules even if you don't need/use them

I didn't know about this.

In that case, we should:

  1. Update the docs
  2. Remove the aliasing code (since it's no longer wanted)

I don't agree that's not a breaking change... but... it's not for me to decide anyway so I'm not discussing it.

@PeterKottas

This comment has been minimized.

Copy link

commented May 31, 2019

Sure, it's not up to me to force or motivate maintainers to remove aliases. I understand the reasons why these were introduced. The way I see it now this is merely an issue. Proper issue title would be "Aliases not working on RNW".

For now, maybe this could be included in known issues section (if there is any).

I wouldn't remove the aliases code yet as like I mentioned before, no official decision has been taken to remove these.

Also I am pretty sure this is fixable. It will just require some clever dynamic imports instead of the code that is there right now.

That being said, I personally am happy with not using aliases. Therefore for my workflow there's no problem atm and I won't be posting the issue or PRs to fix this.

@PeterKottas

This comment has been minimized.

Copy link

commented May 31, 2019

Btw, I don't have access to any statistics but I would assume RNW + styled-components is a fairly exotic use case so I would expect much official support for this as it's probably affecting only a few people.

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

@z0al RNW is pretty specialty and nothing happened in styles components, only RNW Changed and that broke styles components, so of course styles components didn’t update any versions. The problem was caused by RNW. Either don’t upgrade RNW or try to monkey patch in the aliases by changing the styled object exported by styles component or just live without them for now. It’s unlikely this will get fixed in the foreseeable future, so far no one seems to be able and willing, which is ok, this is open source after all.

@probablyup

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

I don't think that was "intentional" since it's a patch release. This is breaking change!

That actually is a breaking change if that's true. I apologize, the core team doesn't make much use of react-native-web so we rely on the community to help with maintenance for that integration. Very open to a followup PR that fixes things satisfactorily.

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

No there is no breaking change on styled-components part. RNW made a new release that broke it's compatibility with styled-components, styled-components didn't do a thing. You could simply not use both libraries together at all. This PR fixed that at least you could use styled(Text) again, but didn't fix the problem that you cannot use styled.Text yet. But nothing was changed in styled-component that broke anything. RNW did change and it was a breaking change, and also an update from 0.10 to 0.11. so everything happened according to normal sem ver. @z0al just thinks that styled-components introduced the problem, when it was RNW.

@z0al

This comment has been minimized.

Copy link

commented May 31, 2019

I apologize, the core team doesn't make much use of react-native-web so we rely on the community to help with maintenance for that integration

It's Ok. Thanks.

@z0al just thinks that styled-components introduced the problem, when it was RNW.

I never said that. Again, I'm not going to discuss this matter anymore since the changes have been released already. I will do another PR if I could come up with an improvement.

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

No changes where released to styled components that broke a thing. RNW released breaking changes with a breaking semver

@shubham2385

This comment has been minimized.

Copy link

commented Jun 8, 2019

Hey! I am still facing same issue with latest styled-components version.

"react": "^16.8.6",
"react-native": "^0.59.9",
"styled-components": "^4.3.1"
"react-native-web": "^0.11.4",

../node_modules/styled-components/native/dist/styled-components.native.esm.js
Attempted import error: 'react-native' does not contain a default export (imported as 'reactNative').
@z0al

This comment has been minimized.

Copy link

commented Jun 8, 2019

@shubham2385 the code should work (even with that error) if your code doesn't use aliases

For example, you should NOT use:

const StyledView = styled.View`
   border-color: blue;
`

Instead do:

import { View } from 'react-native';

const StyledView = styled(View)`
   border-color: blue;
`

Good luck

@shubham2385

This comment has been minimized.

Copy link

commented Jun 8, 2019

Thanks @z0al for help, I have solved issue by removing StyleSheet from react-native.

I have change import like this

import * as reactNative from 'react-native';

and change use of StyleSheet on line number 6487

var InlineStyle = _InlineStyle(reactNative.StyleSheet);

Now I can use styled.Text, styled.View direct.

Example:

const StyledView = styled.View`
  background-color: papayawhip;
`;

const StyledText = styled.Text`
  color: palevioletred;
  padding: 15px;
`;
@z0al

This comment has been minimized.

Copy link

commented Jun 8, 2019

Well, then you have to modify your node_modules folder every time you update styled-components, I wouldn't recommend that.

Also, see the comment above about why doing import * as RN from 'react-native' is bad.

I would advise against using aliases for now.

@shubham2385

This comment has been minimized.

Copy link

commented Jun 8, 2019

@z0al I have try as you suggest still getting Attempted import error: 'react-native' does not contain a default export (imported as 'reactNative'). error.

for solved that error I have remove reactNative import and use of reactNative.

import { StyleSheet } from 'react-native';

and remove reactNative from line no. 6504

return styled([alias]);

Example:

const StyledView = styled(View)`
  background-color: papayawhip;
`;

const StyledText = styled(Text)`
  color: palevioletred;
  padding: 15px;
`;

let me know if any one have other solution for fix this issue.

@PeterKottas

This comment has been minimized.

Copy link

commented Jun 8, 2019

Lol, that's some wild stuff you're doing there @shubham2385 . You'll run into some deep trouble soon but I guess that's how you learn. Changing code in node_modules is probably on the top of the list of things never to do with npm.

Follow the suggestions given to you by zOal and it will work just fine.

@z0al

This comment has been minimized.

Copy link

commented Jun 8, 2019

I have try as you suggest still getting Attempted import error: 'react-native' does not contain a default export (imported as 'reactNative'). error.

@shubham2385 Yep, the error will still be there, but your code will work (assuming you removed all aliases). Just ignore it.

Again, you probably don't want to modify the node_modules.

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

@shubham2385 I would try adjusting the styled export in your code to get aliases working. I haven’t tried it. But styled is just a js object so you should be able to import it and then redefine the aliases yourself, importing the aliases. Then your other code should just work as before.

So in your main index.js file try something like

import styled from 'styled-components/native'
import { Text, View } from 'react-native'

styled.View = styled(View)
styled.Text = styled(Text) 

... // whatever you do to set up you app
@MrLoh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

To get rid of the error we should maybe create a native/index.web.js that handles the react-native-web case. But that would be a breaking. Hänge, since unless someone knows how to handle the dynamic import, it would remove styled.Component not only for RNW 0.11 but also any older RNW versions (I just didn’t upgrade so far to avoid this issue).

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

@shubham2385 if you want to really modify your node modules. Use patch package

@probablyup

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

I’ll try to fix this for real in v5 so y’all don’t have to do weird workarounds.

@hushicai

This comment has been minimized.

Copy link

commented Jun 14, 2019

I am sorry, but is there any progress for this problem?

@nathggns

This comment has been minimized.

Copy link

commented Jun 24, 2019

Is this fixed in v5?

@probablyup

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@nathggns nothing has changed yet, but open to PRs. Have you tested with v5 to see if your problem exists? Also conversation about this should move to a new issue for tracking purposes.

@styled-components styled-components locked as off topic and limited conversation to collaborators Jun 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.