mv-init is not applied when storage returns "" (empty string) #438

lukesmurray · 2019-01-10T21:37:00Z

I tried using pre elements to store data live and start data. However, the input field is not initialized unless there is at least one character of whitespace in the HTML element for storing the data, or the HTML element for storing the data is closed immediately i.e. <pre id="foo"/> which is bad. This issue is annoying because my editor automatically removes whitespace between empty tags so I cannot get the initial data to show up.

Here is a codepen replicating the issue. the first mavo app shows the problem, the second mavo app is the expected behavior.

The text was updated successfully, but these errors were encountered:

LeaVerou · 2019-01-12T16:40:31Z

Looks like there's definitely a bug at play here.

Btw:

self-closing a <pre> does not work in HTML (as opposed to X(HT)ML), the / is just ignored. The reason it works for self-closing tags like <img /> is that these don't require a closing tag altogether, i.e. just <img> would work fine, but the slash is useful for humans reading the HTML. You can verify this by typing <pre />foo and observing how the "foo" is preformatted, despite theoretically being outside the <pre>. Or just inspecting it :)
You don't need all this HTML in codepen, the whole point is that you only need to type what you would put between <body>...</body>. The rest is added automatically. If you need any includes, you do that via the settings. Specifically for Mavo demos, you can use http://play.mavo.io which opens Codepen with all that stuff pre-filled.

Here's an even more reduced testcase: https://codepen.io/leaverou/pen/OraoPy?&editors=1100#0

From that, I added two tests in the testsuite: Empty storage , Whitespace storage

After investigating a little bit, it seems that this is actually the case of two bugs cancelling each other out, lol.

The relevant bits in the code are:
https://github.com/mavoweb/mavo/blob/master/src/mavo.js#L476
https://github.com/mavoweb/mavo/blob/master/src/backend.js#L40
https://github.com/mavoweb/mavo/blob/master/src/formats.js#L37

Note that in formats.js, if the string to be parsed is falsy, the entire parse function returns null. However, the string " " would be truthy, so it would be passed on to JSON.parse(), which would in turn error.
Back to mavo.js, note that there's an asynchronous catch() after the call to load() which loads data from the init backend. However, null would not go there, so it would just be rendered as empty data, ignoring the init backend.

The two bugs are:

Whitespace should also return null, not be treated as an error. One easy fix would be to convert to a string and .trim() before anything in parse().
Init should be applied when parsed data is null.

I suspect the fix would be one of the following:

Backend#load() would throw if the data is null, so that init gets a chance
Mavo#load() checks if the data to be rendered is null and if so, and we have an init backend, use the init backend.

The latter is probably conceptually better, but a bit tricky to do in a DRY way.

lukesmurray · 2019-01-14T15:29:07Z

Wow thank you for the in depth explanation and for the tips about html and the code pen. I can try to work on solving this.

lukesmurray · 2019-01-14T18:37:06Z

So I implemented Mavo#load() with a check if data being rendered is null. I refactored loading from the init into a function and then call that function if the data is null or if there has been an error. This isn't terrible but it still repeats itself so I don't think its worth a pull request. I'll put more thought into it.

const load_from_init = async () => {
	// Try again with init
	if (this.init && this.init != backend) {
		backend = this.init;
		return this.init.ready.then(() => this.init.load());
	}

	// No init, propagate error
	return Promise.reject(err);		
}

return backend.ready.then(() => backend.load())
.then(data => {
	// if we get null data try to load from init 
	return data === null? load_from_init() : data;
})
.catch(err => {
	// if we get an error try to load from init
	return load_from_init();
})

Regarding converting to a string I wasn't sure how to do that without introducing new bugs. My first inclination was to use JSON.stringify but this doesn't help since it maps whitespace ' ' to whitespace with quotes "' '".

LeaVerou · 2019-01-14T22:37:45Z

Thanks for working on it!
Do note that underscore_case is rare in JS, since the convention of the language itself is camelCase.
One thing that will be useful here is that every promise is resolved after a .catch(). So we could have the init loading in one place, and just return null from catch() if there's an init backend.
Do make sure to maintain that if there's no init, the error should be propagated (like it currently is).
On another note, don't be afraid to send a PR that needs corrections. We can iterate on it more easily with code reviews. :)

Regarding converting to a string, the safest way is just + "". However, it's not as trivial as just doing that, because things like null, false, 0 should continue to be falsy. Perhaps something like serialized && (serialized + "").trim().

lukesmurray · 2019-01-14T22:54:49Z

Thank you for the pointers. I really appreciate it. I'll see if I can put all of this together into a pull request.

LeaVerou added a commit to mavoweb/test that referenced this issue Jan 12, 2019

Add backend tests for mavoweb/mavo#438

2ddf55e

LeaVerou added the bug label Jan 12, 2019

LeaVerou changed the title ~~Initial data doesn't populate inputs with html element storage~~ mv-init is not applied when storage returns "" (empty string) Jan 12, 2019

lukesmurray linked a pull request Jan 15, 2019 that will close this issue

Fix mv-init is not applied when storage returns "" #442

Open

LeaVerou added this to the v0.2.1 milestone Feb 10, 2019

DmitrySharabin modified the milestones: v0.2.1, v0.4.0 Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

mv-init is not applied when storage returns "" (empty string) #438

mv-init is not applied when storage returns "" (empty string) #438

lukesmurray commented Jan 10, 2019 •

edited

Loading

LeaVerou commented Jan 12, 2019 •

edited

Loading

Uh oh!

lukesmurray commented Jan 14, 2019

Uh oh!

lukesmurray commented Jan 14, 2019

Uh oh!

LeaVerou commented Jan 14, 2019

Uh oh!

lukesmurray commented Jan 14, 2019

Uh oh!

mv-init is not applied when storage returns "" (empty string) #438

mv-init is not applied when storage returns "" (empty string) #438

Comments

lukesmurray commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LeaVerou commented Jan 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukesmurray commented Jan 14, 2019

Uh oh!

lukesmurray commented Jan 14, 2019

Uh oh!

LeaVerou commented Jan 14, 2019

Uh oh!

lukesmurray commented Jan 14, 2019

Uh oh!

lukesmurray commented Jan 10, 2019 •

edited

Loading

LeaVerou commented Jan 12, 2019 •

edited

Loading