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

Feat: notify users whenever an update available #129

Merged
merged 3 commits into from
Jun 6, 2022
Merged

Feat: notify users whenever an update available #129

merged 3 commits into from
Jun 6, 2022

Conversation

huynhducduy
Copy link
Contributor

@huynhducduy huynhducduy commented May 30, 2022

Features

About @duong-le's considerations in #109 (comment)

  1. update-notifier already support 3 ways to disable the notification, I don't think that we need more than that.
  2. As the package author said: This behavior is intentional in Notification is only displayed at most once per interval sindresorhus/update-notifier#209 (comment), the choice is up to us.
  3. update-notifier has a helpful distTag support so we can take advantage of it to notify in multiple distribution types.

Screen Shot 2022-05-30 at 06 50 41

@netlify
Copy link

netlify bot commented May 30, 2022

Deploy Preview for jest-preview-library ready!

Name Link
🔨 Latest commit 6ef680a
🔍 Latest deploy log https://app.netlify.com/sites/jest-preview-library/deploys/629d479385a2280008b3a119
😎 Deploy Preview https://deploy-preview-129--jest-preview-library.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Owner

@nvh95 nvh95 left a comment

Choose a reason for hiding this comment

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

Hi @huynhducduy. Thank you so much for your interest in Jest Preview and your efforts you put to ship this feature. I left some comments, please let me know your thoughts.

I can't wait to welcome you to contributors.

cli/server/previewServer.js Outdated Show resolved Hide resolved
cli/server/previewServer.js Outdated Show resolved Hide resolved
cli/server/previewServer.js Outdated Show resolved Hide resolved
cli/server/previewServer.js Outdated Show resolved Hide resolved
cli/server/previewServer.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
cli/server/previewServer.js Outdated Show resolved Hide resolved
cli/server/previewServer.js Outdated Show resolved Hide resolved
@huynhducduy
Copy link
Contributor Author

Hi @nvh95, I updated the PR, please take a look.

@nvh95
Copy link
Owner

nvh95 commented May 30, 2022

@huynhducduy Hey Duy, I'm going to do some smoke test and review the PR one more time and let you know if it good to be merged. Thank you for your time. <3

@nvh95
Copy link
Owner

nvh95 commented Jun 4, 2022

@huynhducduy
I am experiencing this issue when running jest-preview.

/Users/my_name/jest-preview/cli/index.js:19
const chalk = require('chalk');
              ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/my_name/jest-preview/node_modules/chalk/source/index.js from /Users/my_name/jest-preview/cli/index.js not supported.
Instead change the require of /Users/my_name/jest-preview/node_modules/chalk/source/index.js in /Users/my_name/jest-preview/cli/index.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/my_name/jest-preview/cli/index.js:19:15) {
  code: 'ERR_REQUIRE_ESM'
}

How to reproduce

cd examples/vite-react
npm install
npm run jest-preview

The reason that chalk@5 dropped support for CommonJS. jest-preview, on the other hand, isn't ready to ship to ESM.
Reference: https://github.com/chalk/chalk#install

IMPORTANT: Chalk 5 is ESM. If you want to use Chalk with TypeScript or a build tool, you will probably want to use Chalk 4 for now.

If you run npm ls chalk in the root folder, you can see most of the installed packages depend on chalk@4.1.2
chalk version 4 is dependencies of most packages

So I would like to ask you to install chalk@4 for two reasons

  • The CLI wouldn't break
  • The package-lock.json doesn't need many changes as now

package-lock.json has 1018 line of changes

Copy link
Owner

@nvh95 nvh95 left a comment

Choose a reason for hiding this comment

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

I did some tests and it work great. Please help to resolve 2 things and this PR will be ready to be merged:

  • Run npm run prettier:fix
  • Use chalk@4 instead (see above comment).

Thank you very much for your help @huynhducduy

cli/index.js Outdated
Comment on lines 28 to 36
notifier.notify({
defer: true, // Try not to annoy user by showing the notification after the process has exited
message:[
`${chalk.blue('{packageName}')} has an update available: ${chalk.gray('{currentVersion}')} → ${chalk.green('{latestVersion}')}`,
`Please run ${chalk.cyan('`{updateCommand}`')} to update.`,
].join('\n')
});
Copy link
Owner

Choose a reason for hiding this comment

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

Can you reformat your code by npm run prettier:fix?
You can leave the changes in other files, just need to update code for cli/index.js is sufficient.

Copy link
Owner

@nvh95 nvh95 Jun 4, 2022

Choose a reason for hiding this comment

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

@huynhducduy
Actually, I think your current code is more readable than the one prettier suggests, you can ignore line 30, 31 if you line.
Something like this

<!-- prettier-ignore-start -->
      `${chalk.blue('{packageName}')} has an update available: ${chalk.gray('{currentVersion}')}${chalk.green('{latestVersion}')}`,
      `Please run ${chalk.cyan('`{updateCommand}`')} to update.`,
<!-- prettier-ignore-end -->

It's your call. I am good with either option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nvh95 unfortunately, prettier-ignore-start only works in markdown 😢 , so it will need 2 piece of // prettier-ignore for those 2 lines if i wanna keep the code in my way, and it does not seem so good 😢

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 and my team dropped prettier also because of problems like that, although prettier is simple, it's too opinionated, and sometimes we don't need to strictly format our code like that. IMO, eslint's stylistic rules are enough to play with.

Copy link
Owner

@nvh95 nvh95 left a comment

Choose a reason for hiding this comment

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

LGTM!

@nvh95 nvh95 merged commit f5d29fa into nvh95:main Jun 6, 2022
@nvh95
Copy link
Owner

nvh95 commented Jun 6, 2022

@all-contributors please add @huynhducduy for code

@allcontributors
Copy link
Contributor

@nvh95

I've put up a pull request to add @huynhducduy! 🎉

@nvh95
Copy link
Owner

nvh95 commented Jun 6, 2022

@huynhducduy Thank you very much for your help. Welcome to contributors!

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.

Notify that a new version is available
2 participants