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

Rudimentary fs.watch usage in filesystemadaptor #176

Merged
merged 3 commits into from
Oct 12, 2013
Merged

Rudimentary fs.watch usage in filesystemadaptor #176

merged 3 commits into from
Oct 12, 2013

Conversation

natecain
Copy link
Contributor

Will only watch for changes on wiki files loaded during startup or created by runtime
(Does not yet track renames/deletes)
(Will not notice new files)
May become confused by directory structure changes on some platforms
(see node docs on fs.watch for caveats!)

  Will only watch for changes on wiki files loaded during startup or created by runtime
    (Does not yet track renames/deletes)
    (Will not notice new files)
  May become confused by directory structure changes on some platforms
    (see node docs on fs.watch for caveats!)
@natecain
Copy link
Contributor Author

For now this is just enough to not have to restart the wiki when editing content directly in files.

This is something of an intermediate stop-gap solution for a common annoyance. A more complete solution would at least catch new files and renames, which this does not.

I think a "total" solution would probably involve some refactor of boot.js to remove the dependency on the fs module there, and to allow it to load the core through an arbitrarily specified "bootstrap" sync module, moving the $tw.boot.files out of core and into filesystemadaptor, etc. In this way, boot and core could be instantiated without ever assuming the existence of a useful filesystem, and boot wouldn't need to do anything special to support/prepare-for synchronization.

Perhaps this is something to discuss more in the next Hangout that I am able to attend.

if(e === "change") {
var tiddlers = $tw.loadTiddlersFromFile(filename).tiddlers;
for(var t in tiddlers) {
$tw.wiki.tiddlers[tiddlers[t].title] = new $tw.Tiddler(tiddlers[t]);
Copy link
Member

Choose a reason for hiding this comment

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

This is problematical; the tiddlers object is supposed to be encapsulated by the wiki object, and not modified directly. Could we instead call wiki.addTiddler() in the usual way instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jermolene possibly! I started out using the addTiddler method, but I was running into problems where this would cause the sync to be triggered recursively until node crashed. I'll investigate and see if I can work something up. I agree that directly stuffing a new tiddler is less than ideal.

@Jermolene
Copy link
Member

Thanks @natecain. I agree that we need better modularisation around the file system stuff. Annoyingly, it's hard to take the residual bits of fs specific code out of boot.js because we need to use the code before we've loaded plugins.

@natecain
Copy link
Contributor Author

@Jermolene Yup there is certainly a chicken and egg problem there. Ideally, I'd like to see boot stuffs have no dependencies whatsoever, and be given a "trusted" base platform via some form of dependency injection (likely with components cherry picked from core) with just enough there for it to be able to load the full core "somehow" and continue on. (This would probably include just crypto, sync, and process components, ultimately. Maybe some basic user state, as well?)

@Jermolene Jermolene merged commit 2c02b6d into TiddlyWiki:master Oct 12, 2013
@Jermolene
Copy link
Member

Manually merged in 6ea264f

@Jermolene
Copy link
Member

@natecain - did you see this issue in twdev? I think it may be related to this patch.

https://groups.google.com/d/msg/tiddlywikidev/8TQWFNmx1I8/L5WfMbcWV10J

@natecain
Copy link
Contributor Author

Yes, it is certainly related to my patch. Basically, the wiki itself writing to the filesystem will be seen by the adapter as a change on disk, causing the tiddler file to be re-read and passed to addTiddler, which marks the tiddler as dirty causing the file to be written again, which is seen by the adapter as a change...

This situation was part of why I was originally modifying the tiddlers array directly, instead of calling addTiddler. Using addTiddler, I have to suspend and recreate the file-system watcher for a tiddler while it is being written. (This brings up an interesting tangential point about race conditions on sync adapters, but we'll discuss that some other time.)

I'm assuming that on OSX the file-system semantics are behaving a little differently, and am also assuming that my patch is not quite robust enough to account for that difference in semantics. Unfortunately I can't readily test under OSX.

I have a hunch on what the difference might be, so I might be able to propose a speculative fix. I think that putting the call to fs.writeFile under a process.nextTick callback might possibly resolve the issue.... under the assumption that the problem is that on OSX the watcher itself is not actually disposing immediately but is scheduling disposal at the next return to the main event-loop. However I'd need to do more research to be able to back this theory up in any substantive way.

@Jermolene
Copy link
Member

Hi @natecain perhaps the best way to handle it is by the filesystemadaptor comparing the newly read tiddler with that in the store, and only calling addTiddler() if they are different. And for the comparison we really should add Tiddler.compare() to the core.

Jermolene pushed a commit that referenced this pull request Sep 28, 2014
This code was contributed by @natecain and added in #176. It was almost
immediately disabled because of problems in the field.

I’m removing the code now to simplify the adaptor in advance of some
planned refactoring.
@linonetwo
Copy link
Contributor

linonetwo commented Sep 18, 2021

Wow, this looks very simple, I will try to test if this still works in 2021.

If performance is a problem, can I include 3rd.js (which bundles chokidar) in https://github.com/tiddly-gittly/tiddlywiki-plugins/tree/master/plugins/linonetwo/watch-fs into tiddlywiki core? It is around 200kB. Will it be too large?

My attempt to create watch-fs plugin also failed because of an infinite loop in some cases, but that was because I couldn't access tw internal information, I will try to modify the core this time.

@pmario
Copy link
Member

pmario commented Sep 18, 2021

@linonetwo Your source doesn't contain any reference to the original js-code or library that has been used. ... So it's hard to verify the integrity. .. I did find: https://www.npmjs.com/package/chokidar .. which seems to be used by VSCode and others. ..

2013 the / all file watchers where super buggy and did only work for some OSes. ... This may have changed since then.

I think, if the file-watcher will be implemented as a server plugin, there wouldn't be a problem. If some changes in the core are needed to support the plugin IMO it should be worth a try.

@linonetwo
Copy link
Contributor

I didn't found a standard way to package npm dependency into tw plugin, so I now just use rollup to pack all things into 3rd.js using this simple rollup config https://github.com/tiddly-gittly/tiddlywiki-plugins/tree/master/deps

Surely it will lose dep version information...Hope there can be a better framework to build tw plugin.

@pmario
Copy link
Member

pmario commented Sep 18, 2021

TW uses this, to implement 3rd party stuff. eg: https://github.com/Jermolene/TiddlyWiki5/tree/master/plugins/tiddlywiki/codemirror/files

@linonetwo
Copy link
Contributor

This is similar to mine, I was just minify chokidar into 3rd.js , and it has tons of dependencies, so rollup bundle them all together.

@pmario
Copy link
Member

pmario commented Sep 18, 2021

This is similar to mine, I was just minify chokidar into 3rd.js , and it has tons of dependencies, so rollup bundle them all together.

Yes. I saw it, but "tons of dependencies" is the problem here. ... With TW we try to avoid 3rd party dependencies as good as we can. Especially with npm-libraries you have "dependency hell".

The package-lock has about 3500 lines. ... chokidar has 7 dependencies.

  • anymatch has 2 dependencies. normalize-path and picomatch
  • braces has 1 but is 4 levels deep with other dependencies.
  • and so on ...

... That's a pain. ... Tiddlywiki's backwards compatibility is probably the strongest argument to use it. TW5 is 10 years old and you can still open a V5.1.0 wiki without problems.

With every release, we have to make sure, that the dependencies don't break TW. ... If something is part of the core, it's very hard to get rid of it again.

So the file-watcher should be a plugin and only a generic API, that is needed should go into the core.

@linonetwo
Copy link
Contributor

linonetwo commented Sep 18, 2021

Okay, I will try modify some of core API, and try pick up my watch-fs plugin again, instead of trying to put this feature into the core (as I can't let go chokidar).

The first one will be #6047

But as @pmario said, backward compatibility is important, so my deletion to var self = this might not be good?

@linonetwo
Copy link
Contributor

And for the comparison we really should add Tiddler.compare() to the core.

It is implemented in https://github.com/tiddly-gittly/watch-fs/blob/master/src/watch-fs/deep-equal.ts

It is so complex that I can't write it correct without TS.

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.

4 participants