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

Improve the warning given when using channel before it's defined #1515

Merged
merged 4 commits into from
Aug 18, 2017

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Jul 25, 2017

Issue: #1514 fixed an issue where a "fake" channel that threw a warning broke RN. This left us without a warning

What I did

Instead, let's throw directly and check for that in the RN initializer.

How to test

Check RN-vanilla, plus general testing (CI + CRA-ks).

Note

This is targeted to #1514, we should change the target when that is merged.

Sometimes it's possible things grab the channel and don't access it, which might be a problem.

Another approach would just be to log a warning rather than throwing.

@tmeasday tmeasday requested review from ndelangen and shilman and removed request for ndelangen July 25, 2017 06:15
@tmeasday tmeasday changed the base branch from master to 1509-fix-react-native-example July 25, 2017 06:16
@ndelangen
Copy link
Member

LGTM, ready to merge @tmeasday

@ndelangen ndelangen added this to the v3.2.0 milestone Jul 28, 2017
@ndelangen ndelangen self-requested a review July 28, 2017 09:42
@tmeasday
Copy link
Member Author

Merge to master @ndelangen ?

@ndelangen
Copy link
Member

Ow I didn't notice this isn't actually based on master.

In that case, we first need to rebase on master, or merge into it's current base, and proceed from there.

This particular change look good to me, I haven't seen #1509-fix-react-native-example

@tmeasday
Copy link
Member Author

That's been merged I think. Let me mess with the PR

@tmeasday tmeasday changed the base branch from 1509-fix-react-native-example to master July 28, 2017 11:14
@codecov-io
Copy link

codecov-io commented Jul 28, 2017

Codecov Report

Merging #1515 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1515      +/-   ##
==========================================
- Coverage   20.84%   20.82%   -0.02%     
==========================================
  Files         251      251              
  Lines        5575     5579       +4     
  Branches      668      669       +1     
==========================================
  Hits         1162     1162              
- Misses       3900     3903       +3     
- Partials      513      514       +1
Impacted Files Coverage Δ
lib/addons/src/index.js 0% <0%> (ø) ⬆️
app/react-native/src/preview/index.js 0% <0%> (ø) ⬆️

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 afeebe0...983dd74. Read the comment docs.

@ndelangen ndelangen merged commit 45ec15e into master Aug 18, 2017
@ndelangen ndelangen deleted the addon-channel-warning branch August 18, 2017 12:32
@ndelangen ndelangen changed the title Addon channel warning Improve the warning given when using channel before it's defined Aug 18, 2017
tmeasday added a commit that referenced this pull request Aug 22, 2017
We need to check for the channel exception on both sides of the channel.
shilman added a commit that referenced this pull request Aug 22, 2017
@usulpro
Copy link
Member

usulpro commented Aug 23, 2017

@tmeasday
Copy link
Member Author

@usulpro -- why does it fail? In what context? Storyshots?

@usulpro
Copy link
Member

usulpro commented Aug 23, 2017

@tmeasday it fails if I run Jest test which imports this file. Actually I already have fixed this just by removing this line inside the not covered code

@tmeasday
Copy link
Member Author

A Jest test -- is it storyshots or something else?

If something else perhaps it needs to setup a fake channel like SS does?

@usulpro
Copy link
Member

usulpro commented Aug 24, 2017

I faced it in #1501 (addon-info)
But the easiest way to reproduce it -- just insert const channel = addons.getChannel(); in *.test.js and run Jest.
Actually, I agree - fake channel it the best and right way to deal with it. In my case, I don't test part of code related to channel since it's a very small piece of code so I left it uncovered. Maybe I should write a test for that later.

@tmeasday
Copy link
Member Author

Yeah i'm not sure. I couldn't figure out a better way to warn people about the addons.getChannel() = undefined problem (which otherwise leads to a vanilla "can't access X on undefined" issue), and how they might solve it

Fake/mock channel seems pretty reasonable. With jest you can do it with one line I reckon.

@ndelangen
Copy link
Member

With 4.0 I will change channel to be a Promise

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