Skip to content

fix: remove update-notifier as unhelpful #4856

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

Merged
merged 2 commits into from
Feb 9, 2023
Merged

fix: remove update-notifier as unhelpful #4856

merged 2 commits into from
Feb 9, 2023

Conversation

danez
Copy link
Contributor

@danez danez commented Feb 8, 2023

Ref netlify/cli#4921

I wanted to update update-notifier from 5.0 to 6.0 to fix some security warnings in its sub-dependencies. While that worked flawlessly, I started to question what the purpose of this update-notifier in build was doing and came to the conclusion, not much. Even worse it might show an update notice for users that they cannot fix.

Currently build shows this update notice when all these 3 things are true:

  1. @netlify/build runs in the Netlify CLI
  2. an error happens
  3. @netlify/build is not up2date

This can happen when the CLI is outdated, but the CLI already uses update-notifier, so there would be two warnings about the same thing.

This can also happen if the CLI is at the latest version but @netlify/build has just release a new version and it is not included in the CLI yet. In this case, the user cannot do anything about this, because the CLI has a shrinkwrap file and we force the version of @netlify/build, so updating the lockfile in the users environment will not update @netlify/build.

For this reason I though I propose to completely remove update-notifier here. I hope I did not miss anything, otherwise, I'm happy to revert and update instead.

A takeaway/followup from this for the CLI: In the CLI we only do the update check every 12 hours. We could change that, so that it resets the interval when an error happens? So assuming we shipped something that always throws an error in the build command, then we would want to notify the user as soon as possible about the new version.


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures
    we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
    something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

istockphoto-1018078858-612x612

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
danez Daniel Tschinder
Currently build shows a warning when run in the Netlify CLI and `@netlify/build` is not
up2date. This can happen when the CLI is outdated, but the CLI already uses update-notifier,
so there would be two warnings. This can also happen if the CLI is latest but @netlify/build
hasn't just release a new version and it is not included in the CLI yet. In this case the user
cannot do anything about this, because the CLI has a shrinkwrap and we force the version of
@netlify/build, so updating the lockfile will not help.
@danez danez requested review from eduardoboucas, lukasholzer and a team February 8, 2023 21:37
@danez danez self-assigned this Feb 8, 2023
@danez danez requested a review from a team February 8, 2023 21:38
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@merlyn-at-netlify
Copy link

Kudos for addressing the dependency issue, and yet more kudos for determining it should be removed all together!

@@ -2,7 +2,7 @@ import { NetlifyConfig } from '../../types/index.js'

export type Mode = 'buildbot' | 'cli' | 'require'

export type BuildCLIFlags = {
export interface BuildCLIFlags {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you convert it to an interface?

Types have the big advantage that you can build easy joined types.

interface Otherthing extends BuildCLIFlags, OtherType {}

// vs

type Otherthing = BuildCLIFlags & Omit<OtherType, 'someProp'>

So the type offers more flexibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I usually just follow what the TS docs mention:

For the most part, you can choose based on personal preference, and TypeScript will tell you if it needs something to be the other kind of declaration. If you would like a heuristic, use interface until you need to use features from type.

but reverted

Verified

This commit was signed with the committer’s verified signature.
danez Daniel Tschinder
Copy link
Contributor

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

LGTM!

@danez danez merged commit 418914d into main Feb 9, 2023
@danez danez deleted the update-notifier branch February 9, 2023 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants