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.

Sign up for GitHub

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

Jump to bottom

[WIP] core: Use AtomWatcher on macOS #1677

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

[WIP] core: Use AtomWatcher on macOS #1677

wants to merge 6 commits into from

Conversation

sebn
Copy link
Contributor

@sebn sebn commented Jul 12, 2019

So we can:

  • Drop the whole chokidar namespace (sort of)
  • Get rid of the 10s delay
  • Lower resources consumption
  • PR is not too big
  • it improves UX & DX in some way
  • it includes unit tests matching the implementation changes
  • it includes scenarios matching a new behaviour or has been manually tested
  • it includes relevant documentation

sebn added 6 commits July 12, 2019 13:55
@sebn
core/local/atom_watcher: Don't check platform in .producer()
69ebb2a
@sebn
test/support/helpers/scenarios: Don't run Windows scenarios on macOS
c7ef0af
@sebn
test/scenarios: Generate atom/darwin captures
09292bc
@sebn
test/unit/config: Fix test descriptions
1c2fb61
@sebn
[FIXME] core/config: Use atom watcher by default on macOS
b9ff4fe 
We probably don't want to do this at first.
Just merge macOS support into master, so we can publish a new release.
And test the new combo by enabling it in config file.
@sebn
[FIXME] core/local/steps/producer: macOS LocalEventBuffer
2b78bac 
Temporary proof-of-concept.
To be reimplemented as a macOS specific step just after producer.
(captures would take the output of this new step)
sebn
sebn commented Jul 12, 2019
View reviewed changes
core/local/atom/producer.js
const timeoutInMs = process.env.NODE_ENV === 'test' ? 1000 : 10000
this.macOSBuffer = new LocalEventBuffer(timeoutInMs, batches => {
for (const batch of batches) {
this.process(batch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

await?

sebn
sebn commented Jul 12, 2019
View reviewed changes
core/local/atom/producer.js
this.watcher = await watcher.watchPath(
this.syncPath,
{ recursive: true },
this.process
onEventBatch
Copy link
Contributor Author

@sebn sebn Jul 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

should we regenerate all captures with buffer? (I believe they were regenerated before reintroducing the buffer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also is the buffer necessary or not?

@taratatach taratatach marked this pull request as draft August 27, 2020 09:15
@taratatach taratatach force-pushed the master branch 2 times, most recently from 8a36ecb to a9a996e Compare April 10, 2021 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@sebn