Discussion: [js-flipper] allow more configuration for device - Githubissues

facebook / flipper

A desktop debugging platform for mobile developers.

https://fbflipper.com/

MIT License

13.34k stars 953 forks source link

Discussion: [js-flipper] allow more configuration for device #3887

Open usrbowe opened 2 years ago

usrbowe commented 2 years ago

Current state

I would like to discuss more about more flexibility in configuring those fields, when using js-flipper:

[ ] os - currently relies on detectOS util, which might not cover all use cases https://github.com/facebook/flipper/blob/6dc85f6646ba5951cb9e87d085ceb13140673870/js/js-flipper/src/util.ts#L51-L63
[ ] device - uses detectDevice util. Currently also default to undefined as string in clientQuery if unsupported device is used. https://github.com/facebook/flipper/blob/6dc85f6646ba5951cb9e87d085ceb13140673870/js/js-flipper/src/util.ts#L65-L70

Proposal

Specify manually `os`, `device`

This could be done as extra param in flipperClient.start(...): https://github.com/facebook/flipper/blob/6dc85f6646ba5951cb9e87d085ceb13140673870/js/js-flipper/src/client.ts#L150-L151

Use `device` instead of applying `clientQuery.app`

Currently uses the device and client as same name. https://github.com/facebook/flipper/blob/6dc85f6646ba5951cb9e87d085ceb13140673870/desktop/flipper-server-core/src/comms/ServerController.tsx#L258-L264

Allow specify `icon` for app

Currently all js-flipper connection will not have any icon, since it's not specified. We could allow to pass extra clientQuery to provide icon name. Likely could have default for Browser and NodeJS in similar fashion as detectOS util. https://github.com/facebook/flipper/blob/6dc85f6646ba5951cb9e87d085ceb13140673870/js/js-flipper/src/client.ts#L230

Visual comparison

We could go from this state:

To this state:

aigoncharov commented 2 years ago

Love the improvements! I'd be happy to accept PRs for each one of them

usrbowe commented 2 years ago

Ok, let me compile the changes in PR.