-
Notifications
You must be signed in to change notification settings - Fork 178
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
Fix mv-init is not applied when storage returns "" #442
base: main
Are you sure you want to change the base?
Conversation
src/formats.js
Outdated
@@ -34,7 +34,8 @@ var base = _.Base = $.Class({ | |||
var json = _.JSON = $.Class({ | |||
extends: _.Base, | |||
static: { | |||
parse: serialized => Promise.resolve(serialized? JSON.parse(serialized) : null), | |||
parse: serialized => Promise.resolve(serialized && (serialized + "").trim() ? | |||
JSON.parse((serialized + "").trim()) : null), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if the extra conversion to a string is necessary here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not, JSON.parse()
converts to a string internally anyway. You can quickly verify this by typing e.g. JSON.parse(false)
in the console. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Luke!
Thanks for contributing!
The issue here is that if there's no init backend, this will still propagate as an error, but it is not. Having no data is a normal initial state of any app.
I think a better process would be to return null from .catch()
when there is an init backend and in a subsequent .then()
load from the init backend if data is null. What do you think?
Btw, very minor, but you may want to set your editor to remove trailing whitespace, I see there's a tab added in the end of line 490.
src/mavo.js
Outdated
.catch(err => { | ||
// Try again with init | ||
// if there is an init backend then return null and don't propagate the error | ||
if (this.init && this.init != backend) { | ||
backend = this.init; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we need to change the backend variable here.
Sorry I committed early I still haven't fixed the issue 👎 |
Ok so I think this fulfills what you were asking for but the error is still propagated. |
Yes. It's not an error, most apps start with no data! If there's an init backend we load from there, if not, no problem, we just render null! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Luke,
This is not quite what I suggested. As you point out, it still throws an error when the data is null which propagates further if there's no init backend. As I explained in my comment, the data being null is not an error condition.
A few notes that may be helpful:
- We want the init backend to be used in two cases: a) When there is an error and b) when the data is null (which is not an error and should not throw)
- If there's no init backend and the data is null, we just render null, no problem.
- Right now (before any PRs), the init backend is only utilized in a
.catch()
, i.e. when an error has occurred and nulls are treated as errors which are just caught in thatcatch
or propagated and caught in the next one. Do note that this is unavoidable for some backends, notably file-based ones, since the file will often not be found when there's no data yet and will yield an HTTP 404 error (which is special cased later on in this method). - Do note that even when there's an actual error that we display to the end user, we still render null.
Hi Luke, Cheers, |
87087a1
to
20ce135
Compare
@LeaVerou better late than never. This passes the tests you wrote. I did what you suggested.
I can squash these commits if you want me to. |
This is to fix #438. I believe this works and is DRY. My only gripe is that I used
serialized + ""
in two places. If you want I can extract that into a variable.