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

investigate whether lix fs watcher can expose "triggeredBySelf" property #1945

Open
samuelstroschein opened this issue Dec 24, 2023 · 7 comments
Assignees

Comments

@samuelstroschein
Copy link
Member

samuelstroschein commented Dec 24, 2023

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
Copy link
Member Author

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
}})

@janfjohannes
Copy link
Contributor

janfjohannes commented Jan 3, 2024

@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 })

@samuelstroschein
Copy link
Member Author

samuelstroschein commented Jan 3, 2024

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.

@janfjohannes
Copy link
Contributor

janfjohannes commented Jan 3, 2024

@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.

@samuelstroschein
Copy link
Member Author

@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].

@janfjohannes
Copy link
Contributor

janfjohannes commented Jan 5, 2024

@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.

@samuelstroschein
Copy link
Member Author

@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

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

No branches or pull requests

2 participants