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

Adding apollo dev tools. #298

Merged
merged 19 commits into from
Jan 30, 2019
Merged

Adding apollo dev tools. #298

merged 19 commits into from
Jan 30, 2019

Conversation

Gongreg
Copy link
Collaborator

@Gongreg Gongreg commented Dec 13, 2018

Hey, @jhen0409, I wasn't sure how to get in contact with you to discuss apollo dev tools integration, so I will just open the pull request here.

I wanted to talk with you about how we could do a proper integration.

Currently what I do is I add some apollo code that is waiting to get apollo client in the app inside the worker which runs actual app code.
I also add apollo tab inside dev tools.

I think it could be possible not to leak any apollo code to react native debugger by using importScripts command?

Maybe we could have a chat in slack or discord to fully discuss this thing?

@jhen0409
Copy link
Owner

Hi, great to see you're trying to implement this!

Initially I hope it will be included in tabs of remotedev-app (#177 (comment)), but currently it looks good to me, I love it to keep simple, included in devtools tab will be more useful.

I just worried about some performance issues for this implementation, we can try improving it in the future.

I think it could be possible not to leak any apollo code to react native debugger by using importScripts command?

If possible, we could try to import the backend bundle, or just install apollo-client-devtools as dependency.

@Gongreg
Copy link
Collaborator Author

Gongreg commented Dec 17, 2018

Hey @jhen0409,
Yea, the timeout trying to get apollo client isn't the best solution, but it is the same implementation in the dev tools themselves. It would be better if the apollo client itself emitted an event, but that would require updating apollo client. Maybe it is a valid feature for the future.

I will take a look how much has to be changed to support apollo dev tools backend out of the box for the debugger.

I hope to get back with some news sometime this week :)

…connecting the debugger.

Only displays apollo tab if apollo is added inside the project.
@Gongreg
Copy link
Collaborator Author

Gongreg commented Dec 18, 2018

Hey @jhen0409,
I've been looking into things and we should be able to install apollo-client-devtools as dependency. I asked apollo team if they could publish an npm package for it.

This way we will only have to add minimal amount of code inside the react native debugger itself.

Mainly we will have to add code inside app/middlewares/debuggerAPI.js to support communication between apollo devtools and rn webworker and some code inside webworker itself to connect to apollo client in the app itself.

Maybe I will add most of the code in webworker to apollo dev tools themselves, so we could import instead of having it in this project. I will talk about it with someone from apollo team.

Hope to have some more news in next few days

@Gongreg
Copy link
Collaborator Author

Gongreg commented Jan 9, 2019

Hey @jhen0409, I am still waiting from apollo side to publish dev tools as a npm module. When it is published we can move forward.

@@ -24,7 +24,8 @@
"__PLATFORM__",
"__REPORT_REACT_DEVTOOLS_PORT__",
"__FETCH_SUPPORT__",
"__NETWORK_INSPECT__"
"__NETWORK_INSPECT__",
"__APOLLO_DEVTOOLS_SHOULD_DISPLAY_PANEL__"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apollo dev tools are not displayed in the chrome dev tools by default.
It tries to check if window.apolloClient is set. Because we don't have apollo client in window (it is in web worker), I've created a separate bool for it.

@@ -32,6 +32,18 @@ let socket;

const workerOnMessage = message => {
const { data } = message;

if (data && data.source === 'apollo-devtools-backend') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we get at least one message from apollo backend (the code in the worker) it means that apollo client is being used in the app. So we tell apollo dev tools to display its panel in chrome dev tools.

Also we forward the message from webworker to react native debugger web part.

@@ -43,14 +55,22 @@ const workerOnMessage = message => {
socket.send(JSON.stringify(data));
};

const onWindowMessage = e => {
const {data} = e;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we receive data from panel in web part, we forward the message to the worker. There are some checks for payload, but they are there since apollo dev tools don't strictly follow api for event emitter.

@@ -51,6 +53,46 @@ const setupRNDebugger = async message => {
checkAvailableDevMenuMethods(modules, message.networkInspect);
reportDefaultReactDevToolsPort(modules);
}

const interval = setInterval(() => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apollo client doesn't have any event emitted when it is set up. To overcome that I just poll the client till it appears.
It is not an optimal approach, but simple polling shouldn't be too much overhead


let listener;

const bridge = new Bridge({
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code to forward messages received from post message to event emitter for apollo

@Gongreg
Copy link
Collaborator Author

Gongreg commented Jan 9, 2019

Hey, @jhen0409,
I've cleared up the code and wrote comments about the solution I did.
I am not sure how you feel about the comments in the code itself, if you want to I can add them there.

Now I am just waiting for apollo to publish npm package and answer me one question.

@@ -3,6 +3,24 @@ export const getClearAsyncStorageFn = AsyncStorage => {
return () => AsyncStorage.clear().catch(f => f);
};

export const getSafeAsyncStorage = AsyncStorage => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes Async storage throws errors, we want to silence them

@Gongreg
Copy link
Collaborator Author

Gongreg commented Jan 10, 2019

This is a relevant issue in apollo dev tools apollographql/apollo-client-devtools#160
And also a PR apollographql/apollo-client-devtools#165

electron/extensions.js Outdated Show resolved Hide resolved
@jhen0409 jhen0409 self-requested a review January 27, 2019 12:14
@Gongreg
Copy link
Collaborator Author

Gongreg commented Jan 28, 2019

@jhen0409, and we are ready to go! All the changes from the apollo team were published.

Could you review the code some time in around?

Copy link
Owner

@jhen0409 jhen0409 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, I think it's ready to merge. Thanks!

@Gongreg, I also added you as collaborator.

@jhen0409 jhen0409 merged commit 1cff81b into jhen0409:master Jan 30, 2019
@Gongreg
Copy link
Collaborator Author

Gongreg commented Jan 30, 2019

Awesome @jhen0409, can you ping me when you release an updated version?

Also thank you for adding me as a collaborator, I will try to help from time to time, since I use this debugger daily and suggest it for everyone else :)

@Aleksion
Copy link

Would love to see a release of this. I can't get it to work when building from master, so I'm currently stuck :(

screen shot 2019-01-30 at 3 38 41 pm

@jhen0409
Copy link
Owner

@Gongreg, OK, I'll comment to apollographql/apollo-client#4386 when it is released. :)

@Aleksion, It seems to electron-rebuild issue, you need to ensure electron-named-image is successfully installed. Otherwise you can try switch electron-v4 branch and merge with master because it doesn't require any rebuild.

cryptodev523 pushed a commit to cryptodev523/react-native-debugger that referenced this pull request Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants