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

Handle warn output for packager.list #1994

Closed
wants to merge 21 commits into from
Closed

Conversation

joshuayoes
Copy link
Contributor

@joshuayoes joshuayoes commented Aug 5, 2022

Please verify the following:

  • yarn ci:test jest tests pass with new tests, if relevant
  • README.md has been updated with your changes, if relevant

Describe your PR

Resolves #1993 by parsing spawn output as chunks of strings instead of one joined string

  • Added new spawnChunked tool, to grab command output from a spawned process as a array of string chunks instead of one joined string
  • Refactored list function in packager tool to use dependency injection for services, in order to make it easier to test specific use cases
  • Added tests to ensure that listCmd can handle unexpected yarn list or npm list outputs
  • Added guards to get runtime typechecks of command outputs

@joshuayoes joshuayoes self-assigned this Aug 5, 2022
@joshuayoes
Copy link
Contributor Author

joshuayoes commented Aug 5, 2022

What's Left:

  • More clear error message for for unexpected command output
  • More robust validation for yarn list command output

@joshuayoes joshuayoes changed the title Draft: Handle warn output for packager.list Handle warn output for packager.list Aug 8, 2022
@@ -104,7 +103,6 @@ export default {
// generate the project
startSpinner("Igniting app")
await spawnProgress(log(expoCLIString), {
env: cliEnv,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this argument because it is not currently used in spawnProgress

Comment on lines -33 to -35
const packageListOptions: PackageOptions = {
global: false,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is only used by one function, I moved this option into a default argument on listCmd

Comment on lines -27 to -31
const packageInstallOptions: PackageRunOptions = {
dev: false,
expo: false,
onProgress: (out: string) => console.log(out),
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not deleted, but moved lower in file

@joshuayoes
Copy link
Contributor Author

These changes should be implemented in gluegun instead of ignite: infinitered/gluegun#762

@joshuayoes joshuayoes closed this Aug 10, 2022
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.

"npx ignite-cli doctor" throws error if there is warn output before "npm list" command
1 participant