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

Add --update and --watch options #4

Merged
merged 9 commits into from
Feb 21, 2020
Merged

Conversation

LordotU
Copy link
Contributor

@LordotU LordotU commented Feb 20, 2020

No description provided.

@LordotU LordotU force-pushed the feat/watch-and-update branch from 63ffa55 to cad21c4 Compare February 20, 2020 15:14
@ai
Copy link
Owner

ai commented Feb 20, 2020

@LordotU Why did you choose chokidar? What other alternatives we have? What is the difference between them?

package.json Outdated
@@ -61,5 +63,8 @@
"CLI",
"backtick"
]
},
"peerDependencies": {
Copy link
Owner

Choose a reason for hiding this comment

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

It is better to create separate PR for another changes

Copy link
Contributor Author

@LordotU LordotU Feb 20, 2020

Choose a reason for hiding this comment

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

Fixed

async function updateAndShowSnaphots (print, error, cwd, filter) {
print(chalk.blue('\nUpdating snapshots... \n'))

let { stdout, stderr } = await execCommand('"./node_modules/.bin/jest" -u', {
Copy link
Owner

Choose a reason for hiding this comment

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

npx jest --no-install is a better way since we can’t trust to find jest in ./node_modules/.bin/.

For instance, we print-snapshots can be run from ./test dir. In this case ./node_modules/.bin/jest will not be found.

cwd
})

print(chalk.grey(stdout))
Copy link
Owner

Choose a reason for hiding this comment

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

Here you will lose Jest color output. We need to keep origin colors, because jest -u can fall and colors will help us to find the reasons.

Copy link
Owner

Choose a reason for hiding this comment

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

In Node.js we have std: inherit option to bind executed script to existed stdout/stderr. In this case you will not loose colors.

@LordotU
Copy link
Contributor Author

LordotU commented Feb 20, 2020

@LordotU Why did you choose chokidar? What other alternatives we have? What is the difference between them?

Hi!

Chokidar is very hyped, yes, but I think it is the most maintainable package with pretty API, anymatch pattern support and short graph of deps. Also we have, for example:

and some others, that were updated a year ago

@LordotU LordotU force-pushed the feat/watch-and-update branch from f066efe to b71cd24 Compare February 20, 2020 19:25
@ai
Copy link
Owner

ai commented Feb 20, 2020

What about webpack’s watchpack?

@LordotU
Copy link
Contributor Author

LordotU commented Feb 20, 2020

What about webpack’s watchpack?

When I searched, this package was not even on the second page, cause my query was fs watch , not file watch.
By the way, there is an interesting issue webpack/watchpack#130.
Also, as I understand, watchpack try to watch all files instead of ignored?
If you have a positive usage expirience of it's usage - ok. I've used only chokidar and node-watch

bin.js Outdated

if (arg === '--watch') {
(await watchAndShowSnaphots(print, error, cwd, filter))
.on('error', err => error(err.stack || err))
Copy link
Owner

Choose a reason for hiding this comment

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

Let’s refactor this code to something more readable. We even pass error function there, we do not need to print errors here.

@ai
Copy link
Owner

ai commented Feb 20, 2020

By the way, there is an interesting issue webpack/watchpack#130.

Yeap. I prefer smaller dependency


print(chalk.green('Snapshots updated! \n'))
spawned.on('exit', async () => {
print(chalk.green('\nSnapshots updated! \n'))
Copy link
Owner

Choose a reason for hiding this comment

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

I think we do not need this note

@LordotU
Copy link
Contributor Author

LordotU commented Feb 20, 2020

By the way, there is an interesting issue webpack/watchpack#130.

Yeap. I prefer smaller dependency

https://npm.anvaka.com/#/view/2d/watchpack vs https://npm.anvaka.com/#/view/2d/chokidar

Chokidar is the winner? )

@ai
Copy link
Owner

ai commented Feb 20, 2020

Yeap, let’s use chokidar

let showSnapshots = require('./show-snapshots')

async function updateAndShowSnaphots (print, cwd, filter) {
print(chalk.blue('\nUpdating snapshots... \n'))
Copy link
Owner

Choose a reason for hiding this comment

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

Let’s remove this note as well

if (fs.existsSync(`${ cwd }/.gitignore`)) {
ignored = [...new Set([
...ignored,
...parseGitignore(fs.readFileSync(`${ cwd }/.gitignore`)).map(
Copy link
Owner

@ai ai Feb 20, 2020

Choose a reason for hiding this comment

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

We can believe that cwd will be the root of the project (since the user can run tool from test/ dir)

The only way to find the project root is to find package.json.

bin.js Outdated
@@ -16,16 +20,22 @@ function print (...lines) {

async function run () {
let arg = process.argv[2] || ''
let filter = process.argv[3] || ''
Copy link
Owner

Choose a reason for hiding this comment

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

The filter can be passed before --update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are shure about give ability to set filter before args?
For example, all CLIs that working with paths accept it as last argument

Copy link
Owner

Choose a reason for hiding this comment

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

Yeap. User may call npx print-snapshots server, found problem, press arrow up to load latest command in terminal and add --watch in the end.

By the way, did we forget about changing --help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, i will fix it and update help output. Thank you!

cwd, stdio: 'inherit'
})

spawned.on('exit', async () => {
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need async here? exit callback can’t return a Promise

let chokidar = require('chokidar')
let parseGitignore = require('parse-gitignore')

let updateAndShowSnaphots = require('./update-and-show-snapshots')

let rootPath = path.dirname(require.main.filename)
Copy link
Owner

Choose a reason for hiding this comment

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

require.main.filename will point to /path/to/the/project/node_modules/.bin/print-snapshots.

I think we need pkg-up to find package.json and be sure about the root of the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems, I need to sleep a little :-)

@ai ai merged commit 73d6360 into ai:master Feb 21, 2020
@LordotU
Copy link
Contributor Author

LordotU commented Feb 21, 2020

@ai Thanks for the tips. I was glad to help!

@LordotU LordotU deleted the feat/watch-and-update branch February 21, 2020 18:27
@ai
Copy link
Owner

ai commented Feb 21, 2020

Do you have a Twitter account?

@ai
Copy link
Owner

ai commented Feb 21, 2020

Released at 0.2

@LordotU
Copy link
Contributor Author

LordotU commented Feb 21, 2020

Do you have a Twitter account?

The same as Github - https://twitter.com/LordotU.
It's abandoned and now I'm using it just for reading my feed. )

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.

2 participants