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

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

Open
felixhayashi opened this Issue Oct 14, 2015 · 7 comments

Comments

Projects
None yet
3 participants
@felixhayashi
Copy link
Contributor

felixhayashi commented Oct 14, 2015

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:

  • Chrome Browser
  • latest version of the master branch
    • a fresh TW server edition

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

This comment has been minimized.

Copy link
Contributor

tobibeer commented Oct 14, 2015

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

This comment has been minimized.

Copy link
Contributor Author

felixhayashi commented Oct 14, 2015

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

@felixhayashi felixhayashi changed the title Serious(!) recursion error with empty TW in nodejs mode (no extra plugins) Adding tiddler programmatically without text field causes serious recursion error in nodejs mode Oct 14, 2015

@felixhayashi

This comment has been minimized.

Copy link
Contributor Author

felixhayashi commented Oct 14, 2015

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.

felixhayashi added a commit to felixhayashi/TW5-TiddlyMap that referenced this issue Oct 14, 2015

v0.9.17
Important Bugfixes
----------------------------

* Added guards against
  Jermolene/TiddlyWiki5#2025:
  Adding tiddler programmatically without text field causes serious
  recursion error in nodejs mode.
@pmario

This comment has been minimized.

Copy link
Contributor

pmario commented Oct 15, 2015

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

This comment has been minimized.

Copy link
Contributor

tobibeer commented Oct 15, 2015

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

This comment has been minimized.

Copy link
Contributor Author

felixhayashi commented Oct 15, 2015

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

This comment has been minimized.

Copy link
Contributor

tobibeer commented Oct 15, 2015

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

  • the titles of tiddlers for which the full text has already been requested at the server
  • whether or not the response had any text in it

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.