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

Yarn warnings wrongly treated as errors #2861

Merged
merged 14 commits into from Nov 30, 2020
Merged

Yarn warnings wrongly treated as errors #2861

merged 14 commits into from Nov 30, 2020

Conversation

MrSnix
Copy link
Contributor

@MrSnix MrSnix commented Nov 23, 2020

Changes:

  1. Modified if condition from if (stderr) to if (stderr && !isFalsePositive(stderr))
  2. Added function isFalsePositive(std_err) which takes std as input, split it using \r?\n and run isDeprecatedDependencies(str) on each line.
  3. Added function isDeprecatedDependencies(str) which checks if a given message is a plain warning,
    it's commented to better explains what it does.

Feedbacks ?

It resolves #2836

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Nice update from the previous PR, and useful comments 🙂

I've outlined a bunch of questions/comments below, let me know how you get on or if you have any questions!

packages/insomnia-app/app/plugins/install.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/plugins/install.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/plugins/install.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/plugins/install.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/plugins/install.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/plugins/install.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/plugins/install.js Show resolved Hide resolved
packages/insomnia-app/app/plugins/install.js Outdated Show resolved Hide resolved
@MrSnix
Copy link
Contributor Author

MrSnix commented Nov 24, 2020

Changes:

  1. The regex now is compiled outside the function as global variable
  2. I removed the flags /gm from regex
  3. Modified comments as requested
  4. Warning are now displayed with console.log() (we won't hide them)
  5. Using the dot notation to refers to regex groups
  6. Created install.test.js with some tests which validate the exported isDeprecatedDependencies(str)

Feedbacks ?

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Nice tests; just need to think about naming tests the same way one would think about function names. 🙂

packages/insomnia-app/app/plugins/install.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/__tests__/install.test.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/__tests__/install.test.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/__tests__/install.test.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/__tests__/install.test.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/__tests__/install.test.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/plugins/install.js Outdated Show resolved Hide resolved
packages/insomnia-app/app/plugins/install.js Show resolved Hide resolved
@MrSnix
Copy link
Contributor Author

MrSnix commented Nov 27, 2020

Changes:

  1. isFalsePositive has been renamed to containsOnlyDeprecationWarnings
  2. As requested tests have been provided for containsOnlyDeprecationWarnings
  3. As requested some tests have been unified
  4. Tests now have meaningful names
  5. As requested output on the console has been modified
  6. There is no sixth item in this list

Feedbacks?

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 tested locally with insomnia-plugin-pdf-report (has a warning) and insomnia-plugin-faker (has no warning).

One note about changing console.log to console.warn but rest is good!

@develohpanda develohpanda merged commit 9270fd9 into Kong:develop Nov 30, 2020
1 check passed
@MrSnix MrSnix deleted the bug/yarn-crash-plugin branch December 1, 2020 09:14
@ThomasP1988
Copy link

Hi everyone, I'm using Insomnia v2020.5.2 and i still have this issue:

Error: Yarn error warning insomnia-plugin-aws-cognito-iam > @aws-amplify/auth > amazon-cognito-identity-js > buffer@4.9.1: This version of 'buffer' is out-of-date. You must update to v4.9.2 or newer
warning "insomnia-plugin-aws-cognito-iam > @aws-amplify/auth > @aws-amplify/core > @aws-sdk/client-cognito-identity > @aws-sdk/middleware-retry > react-native-get-random-values@1.5.0" has unmet peer dependency "react-native@>=0.56".

at file:///Applications/Insomnia.app/Contents/Resources/app.asar/bundle.js:103518:16
at ChildProcess.exithandler (child_process.js:295:7)
at ChildProcess.emit (events.js:223:5)
at maybeClose (internal/child_process.js:1021:16)
at Socket.<anonymous> (internal/child_process.js:430:11)
at Socket.emit (events.js:223:5)
at Pipe.<anonymous> (net.js:664:12)

Wasn't it suppose to be fix in the last release?

@develohpanda
Copy link
Contributor

Hi @ThomasP1988, this was merged but not released with 2020.5.2. It will be included with the next release, likely early 2021.

@ThomasP1988
Copy link

Ok thank you, I will wait until there then

@nickroberts
Copy link

nickroberts commented Mar 15, 2021

Still having an issue with this. I'm on the latest, 2021.1.1.

Trying to install insomnia-plugin-reponse.

Error: Yarn error warning insomnia-plugin-response > insomnia-xpath > xmldom@0.1.31: Deprecated due to CVE-2021-21366 resolved in 0.5.0

    at file:///Applications/Insomnia.app/Contents/Resources/app.asar/bundle.js:102243:16
    at ChildProcess.exithandler (child_process.js:295:7)
    at ChildProcess.emit (events.js:223:5)
    at maybeClose (internal/child_process.js:1021:16)
    at Socket.<anonymous> (internal/child_process.js:430:11)
    at Socket.emit (events.js:223:5)
    at Pipe.<anonymous> (net.js:664:12)

@MrSnix
Copy link
Contributor Author

MrSnix commented Mar 15, 2021

This happens because the check was made really strict on purpose.

It passes only if the message is shaped as follows:

[Message format]
Yarn warnings [dependencies-list]:
"The following x is no longer maintained and not recommended for usage,
please upgrade your dependencies."

From the fix:

    message.includes('no longer maintained') &&
    message.includes('not recommended for usage') &&
    message.includes('upgrade your dependencies')

I could make it less strict but there were some concerns that could lead to false positives.

EDIT:
To be more clear, we don't really know what Yarn may emit as a message, it may be a real error or something else like a deprecation warning. Currently, it should fail for any given warning message, so we may implement an array of keywords to match inside the warning message, like "out-of-date" or "deprecated" and so on... but there is always some sort of risks with false positives.

@develohpanda
Copy link
Contributor

develohpanda commented Mar 28, 2021

@nickroberts looking closer at the error you've got, this is actually an issue with a downstream package that Insomnia maintains, being consumed by an external plugin. This should be addressed by an open dependabot ticket and I'll look to get that through soon, you can track it in #3245.

I'm also curious to know why you're installing insomnia-plugin-responses manually? It's bundled within the release by default already and doesn't get impacted by the yarn warning. 😄

In any case, a longer term solution for handling yarn responses is still useful.

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.

Yarn fails plugin installation over a warning (e.g deprecated dependency)
5 participants