[local-cli][packager] Move packager initialization events to reporter #13209

brentvatne · 2017-03-30T02:23:01Z

This PR depends on #13172

Motivation

Packager events are mostly logged through the TerminalReporter by default (#13172 makes this configurable). But there are a few things that aren't passed through TerminalReporter.

We log a banner with some information about the port and what's going on
Also a message about looking for JS files (not sure what that is for / if it is useful beyond telling the user what directory root they started the packager in, but that's another thing).
If the packager fails to start, then we log an error message.

This pull request changes those log messages to be handled by TerminalReporter. I tried to pick good names for the events based on existing names as defined in reporting.

Test Plan

Pull this, run npm start in root and open up UIExplorer. Works as expected, but somehow it just feels better, right?

Next Steps

I'm not very happy about having to include formatBanner from local-cli in packager/src/lib/TerminalReporter, given that packager may split off at some point. I would have just moved it into the packager, but it's a dependency of checkNodeVersion.
Not a huge fan of accessing a private-by-naming-convention property on packagerServer to get the reporter (here). Maybe we should expose a getter for it, or a logEvent function on the packager server instance?
Are there more events that we can send to the TerminalReporter? For example, we internally track the URL of a bundle request, it might be useful to send that along somewhere as well.

…er server

eslint-bot · 2017-03-30T02:32:52Z

packager/src/lib/reporting.js

+  const startedCallback = logReporter => {
+    logReporter.update({
+      type: 'initialize_packager_started',
+      port: args.port,


identifier args Could not resolve name

… log messages too

mkonicek · 2017-03-30T21:33:22Z

Thanks for adding a lot of context! I pinged @davidaurelio and @jeanlauliac on work messenger.

davidaurelio · 2017-03-31T08:09:02Z

lgtm.

I agree with the points you brought up, let’s address them with a follow-up (making .reporter public, moving formatBanner to the packager directory)

facebook-github-bot · 2017-03-31T08:09:35Z

@davidaurelio has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: This PR depends on #13172 Packager events are mostly logged through the TerminalReporter by default (#13172 makes this configurable). But there are a few things that aren't passed through TerminalReporter. - We [log a banner with some information about the port and what's going on](https://github.com/facebook/react-native/blob/8c7b32d5f1da34613628b4b8e0474bc1e185a618/local-cli/server/server.js#L22-L32) - Also [a message about looking for JS files](https://github.com/facebook/react-native/blob/8c7b32d5f1da34613628b4b8e0474bc1e185a618/local-cli/server/server.js#L34-L38) (not sure what that is for / if it is useful beyond telling the user what directory root they started the packager in, but that's another thing). - If the packager fails to start, then [we log an error message](https://github.com/facebook/react-native/blob/8c7b32d5f1da34613628b4b8e0474bc1e185a618/local-cli/server/server.js#L41-L61). This pull request changes those log messages to be handled by TerminalReporter. I tri Closes facebook/react-native#13209 Differential Revision: D4809759 Pulled By: davidaurelio fbshipit-source-id: 2c427ec0c1accaf54bf6b2d1da882cd6bfaa7829

Summary: This PR depends on facebook#13172 Packager events are mostly logged through the TerminalReporter by default (facebook#13172 makes this configurable). But there are a few things that aren't passed through TerminalReporter. - We [log a banner with some information about the port and what's going on](https://github.com/facebook/react-native/blob/8c7b32d5f1da34613628b4b8e0474bc1e185a618/local-cli/server/server.js#L22-L32) - Also [a message about looking for JS files](https://github.com/facebook/react-native/blob/8c7b32d5f1da34613628b4b8e0474bc1e185a618/local-cli/server/server.js#L34-L38) (not sure what that is for / if it is useful beyond telling the user what directory root they started the packager in, but that's another thing). - If the packager fails to start, then [we log an error message](https://github.com/facebook/react-native/blob/8c7b32d5f1da34613628b4b8e0474bc1e185a618/local-cli/server/server.js#L41-L61). This pull request changes those log messages to be handled by TerminalReporter. I tri Closes facebook#13209 Differential Revision: D4809759 Pulled By: davidaurelio fbshipit-source-id: 2c427ec0c1accaf54bf6b2d1da882cd6bfaa7829

Summary: This PR depends on #13172 Packager events are mostly logged through the TerminalReporter by default (#13172 makes this configurable). But there are a few things that aren't passed through TerminalReporter. - We [log a banner with some information about the port and what's going on](https://github.com/facebook/react-native/blob/8c7b32d5f1da34613628b4b8e0474bc1e185a618/local-cli/server/server.js#L22-L32) - Also [a message about looking for JS files](https://github.com/facebook/react-native/blob/8c7b32d5f1da34613628b4b8e0474bc1e185a618/local-cli/server/server.js#L34-L38) (not sure what that is for / if it is useful beyond telling the user what directory root they started the packager in, but that's another thing). - If the packager fails to start, then [we log an error message](https://github.com/facebook/react-native/blob/8c7b32d5f1da34613628b4b8e0474bc1e185a618/local-cli/server/server.js#L41-L61). This pull request changes those log messages to be handled by TerminalReporter. I tri Closes facebook/react-native#13209 Differential Revision: D4809759 Pulled By: davidaurelio fbshipit-source-id: 2c427ec0c1accaf54bf6b2d1da882cd6bfaa7829

[local-cli] Add support for custom log reporter cli option for packag…

b25ebc3

…er server

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Mar 30, 2017

eslint-bot reviewed Mar 30, 2017

View reviewed changes

[local-cli][packager] Use the given log reporter for packager startup…

21c5c60

… log messages too

brentvatne force-pushed the more-packager-events-to-reporter branch from 0ec23e3 to 21c5c60 Compare March 30, 2017 15:41

hramos added the Core Team label Mar 31, 2017

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Mar 31, 2017

facebook-github-bot closed this in b80e2c8 Mar 31, 2017

brentvatne deleted the more-packager-events-to-reporter branch May 13, 2019 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[local-cli][packager] Move packager initialization events to reporter #13209

[local-cli][packager] Move packager initialization events to reporter #13209

brentvatne commented Mar 30, 2017 •

edited

Loading

eslint-bot Mar 30, 2017

brentvatne Mar 30, 2017 •

edited

Loading

mkonicek commented Mar 30, 2017

davidaurelio commented Mar 31, 2017

facebook-github-bot commented Mar 31, 2017

[local-cli][packager] Move packager initialization events to reporter #13209

[local-cli][packager] Move packager initialization events to reporter #13209

Conversation

brentvatne commented Mar 30, 2017 • edited Loading

Motivation

Test Plan

Next Steps

eslint-bot Mar 30, 2017

Choose a reason for hiding this comment

brentvatne Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

mkonicek commented Mar 30, 2017

davidaurelio commented Mar 31, 2017

facebook-github-bot commented Mar 31, 2017

brentvatne commented Mar 30, 2017 •

edited

Loading

brentvatne Mar 30, 2017 •

edited

Loading