monorepo icon indicating copy to clipboard operation
monorepo copied to clipboard

investigate whether lix fs watcher can expose "triggeredBySelf" property

Open samuelstroschein opened this issue 1 year ago • 7 comments

Problem

The fs watching API leads to loops [for example #1929 but many more]

The cycle between 1 and 2 needs to be broken.

1. file watcher -> reloads runtime messages
2. runtime message adjusted -> write to disk  

Proposal

Original thread https://discord.com/channels/897438559458430986/1069566340031066172/1188430377623232605

Investigate whether the lix fs API's watch feature can expose information if the own instance wrote to disk. The information on whether an app's own fs wrote to disk could be used to break the loop without diffing. I expect diffing to be too expensive.

Pseudocode:

fs.watch({ onChange: (change) => {
  if (change.triggeredBySelf){
    // skip
  } else {
    loadMessages()
  }
}})
CleanShot 2023-12-24 at 11 43 10@2x

cc @inlang/lix @martin-lysk

samuelstroschein avatar Dec 24 '23 10:12 samuelstroschein

Interesting. If we can add this to the upcoming lix fs, I would prob even make opting out of own changes the default and listening to own changes opt-in. Developers will surely forget to exclude their own changes from listening until they run into a bug.

fs.watch({ onChange: (change) => {
  // only listens to external changes
}})

fs.watch({ listenToTriggeredBySelf: true, onChange: (change) => {
  // listens to external and triggered by self changes
}})

samuelstroschein avatar Dec 24 '23 11:12 samuelstroschein

@samuelstroschein i think the watch command is the wrong place for this api. my proposal would be to add an option to writeFile to skip triggering a specific watch instance. this will be simple to reason about and also possible to make work with the nodejs fs, although of course only with the lix fs abstraction ( it will be a bit hacky there and probably rely on the mtime of the fs )

const folderWatcher = fs.watch(`/test`, { signal: abortController.signal })


await fs.writeFile(`/test/file1`, textInFirstFile, { skipWatchTrigger: folderWatcher })

janfjohannes avatar Jan 03 '24 13:01 janfjohannes

this will be simple to reason about

Good argument. I add another argument. Skipping watching can now be set for individual write file calls, not entirely enabled or disabled for all file writes.

samuelstroschein avatar Jan 03 '24 13:01 samuelstroschein

@samuelstroschein if you agree i will change the ticket descritpion to implement my proposal. @martin-lysk would be cool to just get a thumbs up if you agree or comment if you have a different view.

janfjohannes avatar Jan 03 '24 19:01 janfjohannes

@janfjohannes I have one hesitation: Would an app even need to listen to its own writeFile actions? Your proposal is an opt-out, which does not seem to make sense.

Every writeFile() call in the inlang SDK would require an abort controller and skipWatchTrigger. This does not seem right. I can't think of a scenario where the SDK would want to react to changes it caused itself. Ideally, own writeFile() calls don't trigger fs.watch(), or the program can manually opt-in to listen to their own changes.

@janfjohannes, I understand your implicit behavior argument. But, opting out of watch triggers instead of opting into receiving them seems to lead to a lot of boilerplate code [that we will get rid of in a v2 breaking change].

samuelstroschein avatar Jan 03 '24 19:01 samuelstroschein

@samuelstroschein of course the standard case is you want all cahnges to an fs no matter where they come from and this is how every other similar feature works, i think everything bseides an opt out will lead to a nightmare.

janfjohannes avatar Jan 05 '24 17:01 janfjohannes

@janfjohannes we should investigate how many fs.writeFile calls we have after #1844 is merged and see whether an opt-out or opt-in is more desirable

samuelstroschein avatar Jan 05 '24 17:01 samuelstroschein

closing because not relevant for lix on sqlite anymore https://opral.substack.com/p/accelerate-by-years-part-iii-lix

samuelstroschein avatar Aug 01 '24 18:08 samuelstroschein