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

[patch] fix node 14 warnings by upgrading prompt dependency #7084

Merged
merged 2 commits into from
Mar 10, 2021
Merged

[patch] fix node 14 warnings by upgrading prompt dependency #7084

merged 2 commits into from
Mar 10, 2021

Conversation

DominusKelvin
Copy link
Contributor

No description provided.

@sailsbot
Copy link

Hi @DominusKelvin! It looks like the title of your pull request doesn’t quite match our guidelines yet. Would you please edit your pull request's title so that it begins with [proposal], [patch], [fixes #<issue number>], [implements #<other PR number>], or [misc]? Once you've edited it, I'll take another look!

@DominusKelvin DominusKelvin changed the title fix: node 14 warnings by upgrading prompt dependency [patch] fix node 14 warnings by upgrading prompt dependency Dec 24, 2020
@sailsbot
Copy link

Thanks for submitting this pull request, @DominusKelvin! We'll look at it ASAP.

In the mean time, here are some ways you can help speed things along:

  • discuss this pull request with other contributors and get their feedback. (Reactions and comments can help us make better decisions, anticipate compatibility problems, and prevent bugs.)
  • ask another JavaScript developer to review the files changed in this pull request. (Peer reviews definitely don't guarantee perfection, but they help catch mistakes and enourage collaborative thinking. Code reviews are so useful that some open source projects require a minimum number of reviews before even considering a merge!)
  • if appropriate, ask your business to sponsor your pull request. (Open source is our passion, and our core maintainers volunteer many of their nights and weekends working on Sails. But you only get so many nights and weekends in life, and stuff gets done a lot faster when you can work on it during normal daylight hours.)
  • make sure you've answered the "why?" (Before we can review and merge a pull request, we feel it is important to fully understand the use case: the human reason these changes are important for you, your team, or your organization.)

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

@eashaw
Copy link
Member

eashaw commented Dec 30, 2020

Hi @DominusKelvin, Would you be able to describe the bug this is fixing in more detail? I want to make sure I'm looking for the right thing when I test this out. Thanks for the PR!

@DominusKelvin
Copy link
Contributor Author

Hi @DominusKelvin, Would you be able to describe the bug this is fixing in more detail? I want to make sure I'm looking for the right thing when I test this out. Thanks for the PR!

@eashaw if you run sails lift on Node.js 14 and above you get a warning about circular dependency. This patch fixes that.

@DominusKelvin
Copy link
Contributor Author

@eashaw here is a Loom video showing the reason why for this Patch
https://www.loom.com/share/afa1f768f29a4847a15b30656e1a4e5b

@DominusKelvin
Copy link
Contributor Author

@eashaw any word on this yet?

package.json Outdated
@@ -54,7 +54,7 @@
"parseurl": "1.3.2",
"path-to-regexp": "1.5.3",
"pluralize": "1.2.1",
"prompt": "0.2.14",
"prompt": "^1.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

Since prompt is an externally maintained dep, lets use a strict semver range 1.1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay that works well

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

Successfully merging this pull request may close these issues.

4 participants