-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Fix crna-kitchen-sink not working #7200
Conversation
This pull request is automatically deployed with Now. Latest deployment for this branch: https://monorepo-git-fork-lonyele-fix-crna-kitchen-sink.storybook.now.sh |
Hey @lonyele, thanks for fixing it! :) I never really use CRNA example, so it keeps getting outdated. We should really improve the situation there, maybe having e2e tests will help. |
app/react-native/package.json
Outdated
"@types/react-native": "^0.57.57", | ||
"react-native": "^0.57.8" | ||
"@types/react-native": "^0.57.64", | ||
"react-native": "^0.59.9" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
our peer dependency still says "react-native": ">=0.57.0"
but it looks like we broke backward compatibility with older version of RN. We may need a deeper look to understand why that happened to see if it can be fixed. With our next major release, we may want to document what version of RN/expo we're going to support and for how long. We should be able to support 0.59 for longer than 0.57 since that's the first version with react hooks support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeap It seems like more investigation is needed. I didn't get much from seeing why the deploy/build failed. For now, I don't know what to do next... If you have a guess, can you tell me where to look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were few lines of changes yet I didn't see this one. :/
What exactly is the reason to currently lock it to .57 ? What breaking change did we introduce?
And do we need to lock it to .59? If there is need we can do it for next major version I guess.
But I don't think that should be the reason why expo build is failing. I could try to take a look into it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main one is some of the storybook core code started using hooks which weren't supported before 59
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know which part? Maybe we can do something about it? Because if we can't change it we have to lock the version and notify people about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure, this is mostly a guess 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gongreg @benoitdion Sorry guys. CI failed because of source-loader problem. After I merged latest next
branch, there was no test fail anymore. Though do you guys think merging this PR is safe? Unless upgrading peer-dependency of react-native
, it looks ok to me. Because only crna-kitchen-sink
gets the upgrade. Not the users using react-native
4fb45a2
to
683fbec
Compare
683fbec
to
e880b3c
Compare
Hm... CI for build and test looks fine but only now deployement has a problem. What should I do? I don't know why now deployment has failed also I can't rerun the ci... btw while I was resolving |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @benoitdion are you ok with this?
I'm ok merging since the peer dependency is version is unchanged but we should still spend time understanding why the bump was required for the kitchen sink app. It could still mean |
@benoitdion I would just list errors that I've met. Most help is got from here
--> Since |
it's likely that we are now incompatible with pre-hook versions of react |
@benoitdion hm.... I revert back |
a528f5e
to
3fa60c5
Compare
683fbec
to
12d3777
Compare
12d3777
to
4c46487
Compare
Issue: #7199
What I did
I've matched
react
,react-native
,expo sdk
version by bumping upreact-native
to latest stable version0.59.9
react-native
to sdk33.0.0
expo
version to33.0.7
expo sdkVersion
to33.0.0
More info
I'm not totally sure but there was a mismatch of
react
,react-native
,expo sdk
version.Possibly introduced from here that upgrading
react
version from16.5.1
to16.8.6
or where
expo sdk
was not upgraded when the upgrade ofreact
andreact-native
has happenedI referenced here, here
I'm not sure about bumping up
expo-cli
or peer-dependencies ofreact-native
to>=0.59
Should I upgrade it?Personally, ugrading
react-native
scares me... so if this breaks something please comment on thisHow to test
If your answer is yes to any of these, please make sure to include it in your PR.