The Wayback Machine - https://web.archive.org/web/20201124044945/https://github.com/facebook/react/issues/20061
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

Open
NickHeiner opened this issue Oct 20, 2020 · 19 comments
Open

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

NickHeiner opened this issue Oct 20, 2020 · 19 comments

Comments

@NickHeiner
Copy link

@NickHeiner NickHeiner commented Oct 20, 2020

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.

@bvaughn
Copy link
Contributor

@bvaughn 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?

@recurx
Copy link
Contributor

@recurx 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 bvaughn commented Oct 21, 2020

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

@recurx
Copy link
Contributor

@recurx 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 bvaughn commented Oct 21, 2020

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

@recurx
Copy link
Contributor

@recurx 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 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 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 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

@NickHeiner NickHeiner commented Oct 21, 2020

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
Member

@gaearon 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 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 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

@NickHeiner NickHeiner commented Oct 21, 2020

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

@minshinkhant minshinkhant commented Oct 23, 2020

Hi, is this issue still opened?

@shaiguelman
Copy link

@shaiguelman 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

@NickHeiner NickHeiner commented Oct 23, 2020

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!

@Fuckmylimpdick

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.