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

imp: Extract debugger-ui #832

Merged
merged 4 commits into from
Nov 14, 2019
Merged

imp: Extract debugger-ui #832

merged 4 commits into from
Nov 14, 2019

Conversation

Esemesek
Copy link
Member

@Esemesek Esemesek commented Oct 30, 2019

Summary:

Extract debugger-ui module out of cli.

TODO:

  • - Cleanup html file (extract styles, scripts and assets)
  • - Improve dev scripts

Next steps:

  • - Rewrite to Typescript
  • - Use React

Test Plan:

  • - Debugger UI working

@grabbou
Copy link
Member

grabbou commented Nov 4, 2019

Thanks for sending this over @Esemesek and happy to see you back with PRs. Just wondering if you have applied the things we have discussed in the office already? I believe we have talked about extracting out the entire debugger middleware out to a separate package.

@Esemesek
Copy link
Member Author

Esemesek commented Nov 6, 2019

@grabbou I extracted the debugger UI as a middleware (https://github.com/react-native-community/cli/blob/debugger-ui/packages/cli/src/commands/server/middleware/MiddlewareManager.ts#L48). Or did you mean to extract the whole MiddlewareManager to a separate package?

EDIT: Discussed it offline, seems okay.

@Esemesek Esemesek marked this pull request as ready for review November 7, 2019 14:09
@Esemesek Esemesek requested a review from grabbou as a code owner November 7, 2019 14:09
@grabbou
Copy link
Member

grabbou commented Nov 14, 2019

Is this PR ready to ship / be reviewed? Also, shall we target next or master with this one? I think there's a little confusion (at least on my end) which branch is the preferred one for breaking changes from now on.

import path from 'path';

export function debuggerUIMiddleware() {
return serveStatic(path.join(__dirname, '..', 'ui'));
Copy link
Member

Choose a reason for hiding this comment

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

nit: ../ui is just fine, path normalises for Windows by default.

@grabbou
Copy link
Member

grabbou commented Nov 14, 2019

Let's merge it to master since it's non breaking.

@grabbou grabbou merged commit 7ead62a into master Nov 14, 2019
@thymikee thymikee deleted the debugger-ui branch February 6, 2020 08:51
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.

3 participants