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

Fix problem with RN on latest master build #3045

Merged
merged 1 commit into from
Feb 21, 2018
Merged

Conversation

tmeasday
Copy link
Member

Issue: I guess RN is broken on master.

I thought I was testing my changes in #3036, but apparently not. What is the latest procedure for install local changes into RN example apps?

What I did

Fixed a bug in the CRNA example app.

How to test

Ensure the RN example apps actually work!

@tmeasday tmeasday requested a review from a team February 21, 2018 11:09
@tmeasday tmeasday changed the title module.hot.dispose is not defined in RN apparently Fix problem with RN on latest master build Feb 21, 2018
@Hypnosphi
Copy link
Member

What is the latest procedure for install local changes into RN example apps?

Unfortunately, you have to do yarn bootstrap --reset --core --reactnativeapp after each change

cc @danielduan

@codecov
Copy link

codecov bot commented Feb 21, 2018

Codecov Report

Merging #3045 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3045   +/-   ##
=======================================
  Coverage   36.87%   36.87%           
=======================================
  Files         417      417           
  Lines        9849     9849           
  Branches      838      821   -17     
=======================================
  Hits         3632     3632           
- Misses       5741     5750    +9     
+ Partials      476      467    -9
Impacted Files Coverage Δ
lib/core/src/client/preview/client_api.js 90.81% <100%> (ø) ⬆️
addons/a11y/src/components/Report/Item.js 0% <0%> (ø) ⬆️
addons/events/src/components/Event.js 37.64% <0%> (ø) ⬆️
lib/ui/src/libs/key_events.js 33.96% <0%> (ø) ⬆️
.../src/modules/ui/components/stories_panel/header.js 29.62% <0%> (ø) ⬆️
lib/core/src/client/preview/story_store.js 84.21% <0%> (ø) ⬆️
lib/core/src/client/preview/syncUrlWithStore.js 39.13% <0%> (ø) ⬆️
...dons/actions/src/lib/types/object/getObjectName.js 62.5% <0%> (ø) ⬆️
addons/viewport/src/components/RotateViewport.js 22.72% <0%> (ø) ⬆️
...src/server/config/WatchMissingNodeModulesPlugin.js 0% <0%> (ø) ⬆️
... and 55 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ab8276...a47b496. Read the comment docs.

@Hypnosphi Hypnosphi merged commit d51be08 into master Feb 21, 2018
@Hypnosphi Hypnosphi deleted the tmeasday/fix-rn branch February 21, 2018 13:34
@tmeasday
Copy link
Member Author

Maybe the --reset part was what I was missing. Does that do the destructive "delete everything" step?

@Hypnosphi
Copy link
Member

Yep

@danielduan
Copy link
Member

RN is a pain to develop on unfortunately because of the many dependencies it needs. I wonder if there's anything we can do to make it easier.

@Hypnosphi
Copy link
Member

There was an attempt in #2375

@ndelangen
Copy link
Member

ndelangen commented Feb 22, 2018

I still have a POC branch that mostly worked RN + yarn workspaces, but it's old. If you want to take a look I can push it.

I lost 5 sanity points working it that

@tmeasday
Copy link
Member Author

It looks like there may have been recent developments within Metro bundler which might? make this better?: facebook/metro#1 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants