TiddlyWiki / TiddlyWiki5

A self-contained JavaScript wiki for the browser, Node.js, AWS Lambda etc.
https://tiddlywiki.com/
Other
8.04k stars 1.19k forks source link

Adding tiddler programmatically without text field causes serious recursion error in nodejs mode #2025

Open felixhayashi opened 9 years ago

felixhayashi commented 9 years ago

Hi @Jermolene,

there seems to be a serious problem with the nodejs syncer when programmatically adding tiddlers and opening them. Can you confirm the recursion bug resulting from the setup below?

edit: as @tobibeer pointed out, this error is caused when the text field is not set when adding a tiddler to the store and then opening it

Setup overview:


The following is tested in chrome, haven't tested other browsers yet.

1) Create a wiki with the following tiddlywiki.info (or just copy/use the TW server edition)
{
  "description": "Basic client-server edition",
  "plugins": [
    "tiddlywiki/tiddlyweb",
    "tiddlywiki/filesystem",
    "tiddlywiki/highlight"
  ],
  "themes": [
    "tiddlywiki/vanilla",
    "tiddlywiki/snowwhite"
  ]
}
2) Start the server
3) Execute the following command via the browser's command line to print tiddler changes to the console:
$tw.wiki.addEventListener("change", function(changedTiddlers) { 
  for(var tRef in changedTiddlers) {
    if(changedTiddlers[tRef].deleted) {
      console.log("Tiddler deleted:", tRef);
    } else {
      console.log("Tiddler modified:", tRef,
                      $tw.wiki.getTiddler(tRef));
    }
  }
});
4) Now execute the following to add a tiddler programmatically
$tw.wiki.addTiddler(new $tw.Tiddler({
  title: "dummy",
  foo: "bar"
}));
5) Now go right to the TW search and search the tiddler "dummy". Click on the link displayed as search result.

See the console messages going crazy!!!

2015-10-14 18 16 42

6) To stop the madness by closing the dummy tiddler in the river.

Everything goes back to normal until you open the tiddler again.


I am a bit buffled by this. Why haven't I noticed this bug before?

-Felix

tobibeer commented 9 years ago

I would imagine the browser thinks he only got a skinny version of this tiddler loaded and so it's asking the server "hey, server, give me the text now, too" via wiki.getTiddlerText() ...and so the server says, "no I don't have any" and so the browser refreshes anyway and then goes again via wiki.getTiddlerText() "hey server, please give me the text, now" and so forth.

To verify if this might indeed be what happens, try adding a text field to your test tiddler and see if it stops going crazy.

Perhaps adding a tiddler to the store in the browser console requires some pushing to the server?

felixhayashi commented 9 years ago

Nice observation @tobibeer. This is exactly what is happening here. Adding a text field, even if empty, will not cause the behaviour above.

felixhayashi commented 9 years ago

Still totally unexpected behaviour from an TW-api users perspective. But at least I can guard against this now by always adding an empty text property when creating tiddlers.

pmario commented 9 years ago

We should probably add some info about mandatory fields to: http://tiddlywiki.com/#TiddlerFields

There are 2 little helper functions: $tw.wiki.getCreationFields and $tw.wiki.getModificationFields that produce the fields, that are needed, if you want a new tiddler to show up in the recent sidebar.

Proposal

We may introduce a new little helper eg: $tw.wiki.getValidTiddlerFields that gives you everything needed for a valid and user visible tiddler.

or addTidder() may check for minimum required fields, which imo are title and text

What do you think?

tobibeer commented 9 years ago

I believe all that is needed for a valid tiddler is a title, not? I don't see why there need to be more strict requirements.

felixhayashi commented 9 years ago

We may introduce a new little helper eg: $tw.wiki.getValidTiddlerFields that gives you everything needed for a valid and user visible tiddler.

I agree with @tobibeer here. TW never required the text field of a tiddler to be set so it makes more sense to fix this in the syncer than to start telling people now they should add some fixer method, this would be a bit strange and non-consistent (what about people that don't do it?).

or addTidder() may check for minimum required fields, which imo are title and text

That would be an option. But preferably this bug should be fixed at the place where it actually occurs: in the syncer mechanism. I mean this only affects nodejs users and is strictly speaking not a TW core problem.

tobibeer commented 9 years ago

What needs to happen here is that the browser module caches...

...and then not ask the server for the full text again untill some polling mechanism or the likes tells the browser that there is new stuff on the server and so the browser module removes those titles from the cache so as to allow to reload them as needed.

Possibly any polling would require refreshes anyway, independent of any "tiddlers-without-text-cache".