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

Bug: Devtools' update-notifier usage is overly eager #20061

Closed
NickHeiner opened this issue Oct 20, 2020 · 24 comments
Closed

Bug: Devtools' update-notifier usage is overly eager #20061

NickHeiner opened this issue Oct 20, 2020 · 24 comments

Comments

@NickHeiner
Copy link

When I run the devtools, I get the update-notifier message:

   ╭────────────────────────────────────────╮
   │                                        │
   │    Update available 4.4.0 → 4.9.0      │
   │   Run npm i react-devtools to update   │
   │                                        │
   ╰────────────────────────────────────────╯

I appreciate the goal of this message. However, the suggested command to run is incorrect for my repo. We use yarn, not npm. And we use a monorepo. This is confusing for devs who are new to the repo.

Would it be possible to suppress the update-notifier message? The team that manages usage of react-devtools in our tool chain is happy to stay on top of updates themselves.

@NickHeiner NickHeiner added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Oct 20, 2020
@bvaughn
Copy link
Contributor

bvaughn commented Oct 20, 2020

This message is not something react-devtools logs. Pretty sure this is just something the npm command does to let you know that you aren't running the latest minor/bugfix version within your current major. I am wrong about this. Looks like we do intentionally notify of an update. I was unaware of that. 😆

Looks like this was added some time ago in facebook/react-devtools#695

Would you be interested in contributing a PR that uses the updateCheckInterval param to put a delay between these checks? Maybe once a week or something?

@bvaughn bvaughn added Component: Developer Tools good first issue and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Oct 20, 2020
@recurx
Copy link
Contributor

recurx commented Oct 21, 2020

Hey @NickHeiner ! If you have not started, can I give it a try? Seems pretty straight-forward to add the check. Should we customise the message too (something more generic) ?

@bvaughn
Copy link
Contributor

bvaughn commented Oct 21, 2020

I don't think anyone has started on this, so please feel free @recurx.

@recurx
Copy link
Contributor

recurx commented Oct 21, 2020

Thanks @bvaughn . updateCheckInterval by default is 1 day. Changing this to 7 days. Should I also change the message to just this:
Screenshot 2020-10-21 at 7 31 09 PM

@bvaughn
Copy link
Contributor

bvaughn commented Oct 21, 2020

I think the message is fine. Any reason to change it?

@recurx
Copy link
Contributor

recurx commented Oct 21, 2020

Was just wondering about this sentence in the issue -

"However, the suggested command to run is incorrect for my repo. We use yarn, not npm. And we use a monorepo. This is confusing for devs who are new to the repo."

@bvaughn
Copy link
Contributor

bvaughn commented Oct 21, 2020

I see.

We could add update the wording to include yarn too I guess. Not sure we should try to cover all possible use cases (like monorepos). The main purpose of this message is to notify people that an update is available, but I think the added action of "run npm i react-devtools" is a nice prompt.

@recurx
Copy link
Contributor

recurx commented Oct 21, 2020

The 'how to install' is a nice prompt. But there would be many different ways people would be using react-devtools. For example if I'm using it globally, I'll have to add a -g, yarn users would need to do yarn add react-devtools . We can do something like this:
Screenshot 2020-10-21 at 8 20 57 PM
There might be more use-cases. Let me know what you think.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 21, 2020

Like I said, I don't think we need to cover all use case 😄 It's nice to mention the most common case or two. That looks fine.

@NickHeiner
Copy link
Author

Yes, @recurx, thanks for pointing out my concern about the message being incorrect. 😄 Changing the message interval doesn't really address the reason I opened this issue.

I agree that it's impractical to cover all use cases. And, frankly, for my users, it's not great to have a message that says something like "use yarn or use npm", because devs in my repo must use yarn.

The ideal solution from my end (and one that should be very easy to implement) is simply to provide an option to suppress showing this update notifier entirely. That option could default to false, and most people wouldn't need to know it existed.

@gaearon
Copy link
Collaborator

gaearon commented Oct 21, 2020

it's not great to have a message that says something like "use yarn or use npm", because devs in my repo must use yarn.

Practically speaking, developers in your repo will encounter many libraries that say npm i something in their instructions. So they need to be aware anyway that npm i something should be substituted by yarn add something in general in your repo. I don't think this is a critical issue in this case.

@NickHeiner
Copy link
Author

NickHeiner commented Oct 21, 2020

I know that many packages use update-notifier, but in this case, we actually don't have many other command line tools printing out install instruction messages. (Off the top of my head, I can't think of a single one.)

And, while I agree that in general, JS devs should have the wherewithal to translate npm commands to yarn commands, this monorepo is contributed to by generalist devs who are not as familiar with the JS ecosystem, and I'm trying to keep things as clean as possible for them.

Is this entire issue critical? No, probably not in any scenario. As a monorepo infrastructure maintainers, would I prefer to have more control over which messages are shown to my dev community? Absolutely. 😄

@bvaughn
Copy link
Contributor

bvaughn commented Oct 21, 2020

Changing the message interval doesn't really address the reason I opened this issue.

While it's not the outcome you're advocating for, I think it is still relevant to what prompted you to open the issue– since it reduces how frequently you'll see this (non-actionable) prompt.

If you would like to submit a PR that adds the ability to permanently opt out of this, you'd be welcome to. It doesn't look like that's something the library we're using provides support for though– and I suspect it's an uncommon enough want that not many people would be interested in doing the leg work but maybe I'm wrong.

@NickHeiner
Copy link
Author

Yes, I agree that reducing the frequency reduces the impact of the issue. 😄

Perhaps I will submit that PR! Thanks for the invitation.

@minshinkhant
Copy link

Hi, is this issue still opened?

@shaiguelman
Copy link

shaiguelman commented Oct 23, 2020

Hi, if possible I would like to tackle this issue. Please let me know if it is still open. Thanks!

@NickHeiner
Copy link
Author

I will hold off to give the other commenters a chance to work on this.

@AmeyaPhadnis-2019H1030012G

Hi, is this issue still open for contribution, I am new to the open-source community and looking forward to opportunities to contribute. Thanks in advance!

@gabrielsanttana
Copy link

gabrielsanttana commented Nov 24, 2020

As @bvaughn said, it would be cool to mention the two most common cases, which in this case would be for npm and yarn.

@abhishekdubey1
Copy link

Hi, is this issue still open for contribution as I intend to work on this?

@CobyCoding
Copy link

CobyCoding commented Dec 12, 2020

I believe this issue has been fixed.

@rakesh456
Copy link

Is this issue fixed?

@mynameisankit
Copy link

Is the issue still unresolved or fixed? I would like to work on this

@bvaughn
Copy link
Contributor

bvaughn commented Jan 25, 2021

I'm going to close this issue because I think the current state of this feature is fine and I have no plans to change it. If someone would like to submit a PR+proposal for a new behavior though, I will review it.

@bvaughn bvaughn closed this as completed Jan 25, 2021
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

14 participants