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

Fix yarn list --json stdOut parsing. fixes #388 #516

Merged

Conversation

asprouse
Copy link
Contributor

@asprouse asprouse commented Jun 14, 2019

What did you implement:

Closes #388

How did you implement it:

Yarn outputs a JSON lines to represent it's output when using the --json flag. This PR simply parses each line and looks for the correct tree JSON line and passes it to the convertTrees step.

How can we verify it:

Run tests or run a build that's using yarn the extra lines are alway output if yarn needs to install anything.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@asprouse asprouse force-pushed the yarn-fix branch 2 times, most recently from 3f19dde to c049836 Compare June 14, 2019 02:30
@asprouse
Copy link
Contributor Author

Sorry had to fixup this PR a few times and had to temporarily disable prettier from .lintstaged because of #515

@asprouse
Copy link
Contributor Author

All rebased on master and ready to review

Copy link
Member

@HyperBrain HyperBrain left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, but finally I got time to review the changes completely.
There might be one issue which changes the behavior of the parsing: If you pass an invalid JSON, previously it rejected with an error and stopped. Now you return null and the deployment will continue but without dependencies, eventually leading to a broken service.
Is this intended, or is there a specific reason for the behavior?

@asprouse
Copy link
Contributor Author

@HyperBrain The JSON parsing is now line by line and some lines may be blank which causes a parsing error. It's be possible to change the code to filter out these blank lines before parsing and to throw on invalid json. I'm fine with either approach if you prefer one over the other. I think that more important point you raise is that we should be throwing a more meaningful error if we cannot find a line where line.type === 'tree' whether this is due to parsing errors or a yarn API change. I am happy to update the PR.

@miguel-a-calles-mba miguel-a-calles-mba added this to the 5.3.3 milestone May 10, 2020
Copy link
Member

@miguel-a-calles-mba miguel-a-calles-mba left a comment

Choose a reason for hiding this comment

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

Looks okay to me.

@miguel-a-calles-mba miguel-a-calles-mba changed the base branch from master to release/5.3.3 May 13, 2020 14:39
@miguel-a-calles-mba miguel-a-calles-mba merged commit 98a9592 into serverless-heaven:release/5.3.3 Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serverless fails to deploy if packager is yarn
3 participants