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

bin.ts refactor for module usability & testing #2601

Closed
paulirish opened this issue Jun 26, 2017 · 2 comments
Closed

bin.ts refactor for module usability & testing #2601

paulirish opened this issue Jun 26, 2017 · 2 comments

Comments

@paulirish
Copy link
Member

the bottom of bin.ts has some core functionality we want to expose. one primary usecase is to get the launch+runlighthouse+getresults flow is available to other modules (see addyosmani/webpack-lighthouse-plugin#5)
other usecase is better tests of the CLI.. these methods arent testable as they sit in bin.ts which requires CLI interaction


Here's the current summary of the two files:

-core/index

  1. throw if missing URL
  2. set logging defaults
  3. generate config from provided json
  4. set up CRI connection
  5. call Runner.run()

-cli/bin.ts

  1. call getFlags() and process early stdout exits
  2. read configPath from disk
  3. set logging defaults
  4. set outputpath defaults
  5. show errors, for a number of cases:
    1. console.error readable error
    2. process.exit w/ code
  6. runLighthouse
    1. call launcher.launch()
    2. call -core/index
    3. call saveResults
    4. handle --interactive mode
    5. kill launcher
    6. call handleError
  7. saveResults
    1. call saveArtifacts (which saves to disk)
    2. call saveAssets (which saves to disk)
    3. prepare filenames
    4. call Printer.write()
    5. call opn()

Ideally much of the items in bin.ts move into index.js. However when you dig into it, this refactor gets a little hairy. For example: handleError both logs a friendly error message and process exits. Half of this is appropriate when LH is a node dependency. Another example: Some webpack-lighthouse-plugin wants to use save-assets and have the report saved to disk but shouldn't get reminded to run LH with the --view flag.

None of this is particularly hard, but it would require time and testing to do it right.

I'm proposing an intermediate fix that will unblock webpack-lighthouse-plugin among others. This fix: move much of bin.ts into its own file that can be safely require()'d without any yargs interference. PR incoming..

@wardpeet
Copy link
Collaborator

@paulirish should we use the projects tab maybe? Or create separate tasks (issues) for this one?

I don't mind giving this my all, not sure what timeframe you're pointing at. Or if i'm the right person to do so 😝

@paulirish
Copy link
Member Author

Fixed by #2602

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants