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

refactor: remove dependency on watchman #192

Merged
merged 1 commit into from
Jul 10, 2018
Merged

refactor: remove dependency on watchman #192

merged 1 commit into from
Jul 10, 2018

Conversation

thsig
Copy link
Collaborator

@thsig thsig commented Jul 3, 2018

Instead of using FB's watchman library to monitor FS changes in watch
commands, we now use the pure-JS library chokidar.

@thsig
Copy link
Collaborator Author

thsig commented Jul 3, 2018

This PR resolves #101.

Copy link
Collaborator

@edvald edvald left a comment

Choose a reason for hiding this comment

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

A few questions but mostly looks good! One other small thing - The watchman dependency still needs to be removed from the homebrew formula template.

src/watch.ts Outdated
res(result)
})
this.watcher = watch(projectRoot, {
ignored: /(^|[\/\\])\../, // just ignore dotfiles for now
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this implicitly ignore files in .gitignore? If not we should probably add that, and could add .gardenignore support along the way (which would work the same).

src/watch.ts Outdated
this.ctx.invalidateCacheUp(cacheContext)

this.ctx.invalidateCache(cacheContext)
this.ctx.invalidateCacheDown(cacheContext)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I don't think you want to call all three of these?

Copy link
Collaborator Author

@thsig thsig Jul 3, 2018

Choose a reason for hiding this comment

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

Ah, this is a leftover from when I was debugging that I seem to have forgotten to clean up (same for the other two below, #192 (comment) & #192 (comment)).

I'll fix these now.

src/watch.ts Outdated
(relPath) => f.name.startsWith(relPath))
if (module.path.startsWith(dirPath)) {
// at least one module's root dir was removed
await this.invalidateCachedForAll()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why invalidate for all?

src/watch.ts Outdated
const changedModule = modules.find(m => filePath.startsWith(m.path)) || null

if (changedModule) {
await this.invalidateCachedForAll()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why invalidate for all?

@edvald edvald changed the title feat: remove dependency on watchman refactor: remove dependency on watchman Jul 10, 2018
Instead of using FB's watchman library to monitor FS changes in watch
commands, we now use the pure-JS library chokidar.
@edvald edvald merged commit 9329e68 into master Jul 10, 2018
@edvald edvald deleted the replace-watchman branch July 10, 2018 11:43
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.

None yet

2 participants