fix: setting background to state mutation violation #858

konrad · 2021-10-16T11:46:11Z

konrad commented

2021-10-16 11:46:11 +00:00

State mutations must be synchronous. Using a promise.then handler to set the background is a violation of that.

dpschen was assigned by konrad

2021-10-16 11:46:11 +00:00

konrad added 1 commit 2021-10-16 11:46:12 +00:00

continuous-integration/drone/pr Build is passing Details

f3c5bbce5a

fix: setting background to state mutation violation

state mutations must be synchronous. Using a promise.then handler to set the background is a violation of that.

konrad requested review from dpschen 2021-10-16 11:46:17 +00:00

dpschen reviewed 2021-10-16 11:49:42 +00:00

src/components/home/contentAuth.vue Outdated

						
				@ -87,3 +87,3 @@

								this.$route.name === 'namespaces.index'

							) {

								this.$store.commit(CURRENT_LIST, null)

								this.$store.dispatch(CURRENT_LIST, null)

dpschen commented

2021-10-16 11:49:42 +00:00

return the result => make async

konrad commented

2021-10-16 11:56:59 +00:00

You mean the whole action?

dpschen commented

2021-10-16 13:09:36 +00:00

Yes. Same reason as in the other answer below.

dpschen commented

2021-10-16 15:44:53 +00:00

Done

dpschen marked this conversation as resolved

dpschen reviewed 2021-10-16 11:50:42 +00:00

src/store/index.js Outdated

						
				@ -65,0 +85,4 @@

						},

					},

					actions: {

						[CURRENT_LIST](ctx, currentList) {

dpschen commented

2021-10-16 11:50:42 +00:00

With [CURRENT_LIST]({state}, currentList) we could keep the diff smaller. But not sure if that makes sense

With `[CURRENT_LIST]({state}, currentList)` we could keep the diff smaller. But not sure if that makes sense

konrad commented

2021-10-16 11:58:49 +00:00

I think it does make sense, changed it.

konrad marked this conversation as resolved

dpschen reviewed 2021-10-16 11:51:11 +00:00

src/store/index.js Outdated

						
				@ -98,4 +122,4 @@

							) {

								if (currentList.backgroundInformation) {

									const listService = new ListService()

									listService.background(currentList)

dpschen commented

2021-10-16 11:51:11 +00:00

Not that this is an action maybe we can already use await here.

dpschen commented

2021-10-16 15:45:06 +00:00

Done

dpschen marked this conversation as resolved

dpschen reviewed 2021-10-16 11:51:45 +00:00

src/views/list/ShowList.vue

						
				@ -143,3 +143,3 @@

								.then(r => {

									this.$store.commit(CURRENT_LIST, r)

									this.$store.dispatch(CURRENT_LIST, r)

									this.setTitle(this.getListTitle(r))

dpschen commented

2021-10-16 11:51:45 +00:00

Really not sure here: should the setTitle await for the action to finish?

konrad commented

2021-10-16 12:01:48 +00:00

I think not. Basically, all that the action does that is async is loading the background. The rest are mutations which happen almost immedieatly (from the user's perspective anyway). I think if the list itself already loaded it is rather confusing if it is waiting for no other reason than to load the list background. Especially since I plan to use BlurHash in the future for list backgrounds where it would not wait until the full background is loaded.

I think not. Basically, all that the action does that is async is loading the background. The rest are mutations which happen almost immedieatly (from the user's perspective anyway). I think if the list itself already loaded it is rather confusing if it is waiting for no other reason than to load the list background. Especially since I plan to use [BlurHash](https://blurha.sh/) in the future for list backgrounds where it would not wait until the full background is loaded.

dpschen commented

2021-10-16 13:08:22 +00:00

Not sure if I got that or if I'm missing something: if you await the dispatch just the setTitle would need to wait. And that's something you could change by reversing the order. The loadList function then has to be async for sure. But since it's not awaited anywhere no code should wait.
The only thing that changes is that the loadList returns a promise that says when it's finished. We don't need that right now, but if we do need it in the future the function is already prepared.

That's nice! BlurHash is really cool :)
Do you know sqip.
No idea how that compares, but I also always wanted to try that.

What I never got with BlurHash is why you would need to implement a new way of hashing images when you can just create a really small image as e.g. webp, inline that and blur it via CSS. Obviously that doesn't work as simple native so there you would need to adapt the implementation, something that BlurHash already does for you.

Not sure if I got that or if I'm missing something: if you await the dispatch just the `setTitle` would need to wait. And that's something you could change by reversing the order. The loadList function then has to be async for sure. But since it's not awaited anywhere no code should wait. The only thing that changes is that the loadList returns a promise that says when it's finished. We don't need that right now, but if we do need it in the future the function is already prepared. ---- That's nice! BlurHash is really cool :) Do you know [sqip](https://www.freecodecamp.org/news/using-svg-as-placeholders-more-image-loading-techniques-bed1b810ab2c/). No idea how that compares, but I also always wanted to try that. What I never got with BlurHash is why you would need to implement a new way of hashing images when you can just create a really small image as e.g. webp, inline that and blur it via CSS. Obviously that doesn't work as simple native so there you would need to adapt the implementation, something that BlurHash already does for you.

dpschen commented

2021-10-16 13:18:14 +00:00

Ahh I just found this note:

blurhash - Really nice, compact placeholder images.
Requires non-native client-side decoding which makes it awkward and slow for browser usage.
Encoding speed is pretty slow (on par with sqip).
Under the hood, the webp format performs a similar set of transforms as the one used by blurhash.

See here: https://github.com/transitive-bullshit/lqip-modern#related

Ahh I just found this note: > blurhash - Really nice, compact placeholder images. > Requires non-native client-side decoding which makes it awkward and slow for browser usage. > Encoding speed is pretty slow (on par with sqip). > Under the hood, the webp format performs a similar set of transforms as the one used by blurhash. See here: https://github.com/transitive-bullshit/lqip-modern#related

konrad commented

2021-10-16 18:35:34 +00:00

sqip looks great! Didn't know it before. I guess I'll do some experiments about the performance of both when it's time to implement it.

🚀 1

konrad added 1 commit 2021-10-16 11:58:39 +00:00