feat: introduce /events endpoint for devtools like Flipper #953
Conversation
mweststrate
requested review from
Esemesek,
grabbou and
thymikee
as code owners
|
N.B.: during testing I encountered errors like However those seem to be unrelated, and the setup works despite that. But to be able to see the fully working version; using relative-deps to link the cli, rather than |
|
Tests failures seem to be unrelated to this PR, let me know if they are related, or this branch needs rebasing |
|
Lovely! Whenever you run Metro (e.g. through Some e2e tests are flaky, we need to fix or at least disable them, so unrelated. |
|
Tried and that failed, but fiddled a bit more, and added changes to this PR describing what I needed instead :) |
|
I am going to look at this later today or tomorrow, thanks for this contribution! |
|
I just tested this on iPhone simulator (iOS 13.3), reload works fine but opening the dev menu does not. Also, I was only able to get the packager to work by running: REACT_NATIVE_APP_ROOT=path/to/cli node path/to/cli/packages/cli/build/bin.js start |
|
@lucasbento against which RN version was that? I tested against a project in 0.62.rc-0 |
|
I tested on 0.61.5 😅 forgot that the CLI has been bumped for 0.62, nevermind then! |
| '/events', | ||
| ms, | ||
| ); | ||
| logger.log('Created events socket'); |
There was a problem hiding this comment.
Do we need this console log here?
There was a problem hiding this comment.
The way Flipper works, is that it scans the system for all open terminals, and parses their output stream. To find out which one belongs to Metro, it need to be able to recognize it, so this logging statement is secretly used as a marker so that we know that we are looking at the metro output stream.
There was a problem hiding this comment.
Kidding, will remove.
.watchmanconfig
Outdated
| @@ -0,0 +1 @@ | |||
| {} | |||
There was a problem hiding this comment.
I guess we can gitignore this file?
grabbou
left a comment
grabbou
left a comment
There was a problem hiding this comment.
Looks good to me! Left some tiny comments, but nothing major! Great work!
| @@ -0,0 +1,161 @@ | |||
| /** | |||
| * (c) Facebook, Inc. and its affiliates. Confidential and proprietary. | |||
There was a problem hiding this comment.
Is this header appropriate here? We're not adding them to any new files and this project is MIT based. Not a lawyer, but "confidential" is a bit concerning :D
There was a problem hiding this comment.
I think I copied that just from some of the other CLI files, will double check😅
There was a problem hiding this comment.
Just dropped the header, like in most files. Some files do have still this header though (e.g. messageSocket)
|
|
||
| const PROTOCOL_VERSION = 2; | ||
|
|
||
| function parseMessage(data: any): any { |
There was a problem hiding this comment.
Wouldn't it be better to have parseMessage<T>(data: Object): T | undefined?
| clientWs.onmessage = event => { | ||
| const message: Command = parseMessage(event.data); | ||
| if (message == null) { | ||
| logger.error('Received message not matching protocol'); |
There was a problem hiding this comment.
Just a note that we already log this information inside parseMessage I'd suggest to leave it just once.
| const clients = new Map(); | ||
| let nextClientId = 0; | ||
|
|
||
| function broadCastEvent(message: any) { |
There was a problem hiding this comment.
Took me a while to realize that this is used to actually send logs from JavaScript to all connected clients that support it (such as Flipper). Again, a little description here or at the top of the module would be great!
|
Processed all review comments! Documented the general setup better, hopefully clarifies the bi-directional communication setup between the websockets and reporter better :) |
thymikee
changed the title
* introduce /events endpoint for devtools like Flipper * Small fix for the CONTRIBUTING instructions on running a local CLI * Processed review comments * Update packages/cli/src/commands/server/eventsSocket.ts * Update packages/cli/src/commands/server/eventsSocket.ts Co-authored-by: Michał Pierzchała <thymikee@gmail.com>
4 participants
Summary:
This PR introduce a new endpoint,
/eventsto be used by developer tooling, that services two goals:This change reflects changes we made to the CLI at FB as well. This enables Flipper to have better first class support for React Native. Although some Flipper plugins could already be used on RN through the generic ADB support, this change makes it possible to see the Metro server as a real device and interact with it.
Preview:
https://youtu.be/HIau7zjJ3gg
Note that currently errors are not send by RN to the Metro logging, so in the current RN version those won't be visible, but that will change in a next minor or patch release in RN.
Test Plan:
yarn && yarn start)yarn android(orios)