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

don't check for hydrationComplete #4076

Closed
wants to merge 2 commits into from
Closed

don't check for hydrationComplete #4076

wants to merge 2 commits into from

Conversation

mslourens
Copy link
Contributor

@mslourens mslourens commented Jan 17, 2022

Description

I removed the check for hydrationComplete since that boolean was set to true the first time the screen was rendered. When clicking the layouts tab and back to the screens tab, the state was not updated by the url and didn't change anymore. After removing the hydrationComplete boolean, the state was updated correctly.

Fixes #4075

Screenshots

image

@codecov-commenter
Copy link

Codecov Report

Merging #4076 (f3bdedd) into develop (4f9ac15) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4076   +/-   ##
========================================
  Coverage    68.91%   68.92%           
========================================
  Files          139      139           
  Lines         4633     4634    +1     
  Branches       685      685           
========================================
+ Hits          3193     3194    +1     
  Misses        1018     1018           
  Partials       422      422           
Impacted Files Coverage Δ
packages/server/src/middleware/currentapp.js 70.37% <100.00%> (+0.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81218cb...f3bdedd. Read the comment docs.

Copy link
Member

@shogunpurple shogunpurple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@aptkingston
Copy link
Member

Hey @mslourens! Sorry it took me so long to get round to looking at this.

While you're right that this does solve the problem, the hydrationComplete check in that layout is actually very important as it prevents endless cycles of update state > update url > update state > update url > ....

I've pushed up a PR here with a safer way to fix this: #4681

It's a bit confusing because we have logic in quite a few different places to update state, but the logic that needed updated in this case was the index.svelte file for this route. This file contains a selectValidAsset function which normally selects an asset of a certain type whenever landing on a URL like .../screen or .../layout (without any ID's after it).

The logic in that function is good, but it doesn't account for when no assets of a certain type exist. In this case, that's highlighted by a new app. If we don't have any screens, then we never updated state, and the builder (and preview) always think that a layout is still selected. That's why the preview app and the builder didn't properly update, as described in your issue.

Hopefully that makes some sense! It took me a while to get to the bottom of this - it's a complicated issue.

@mslourens
Copy link
Contributor Author

Hey Andrew, thanks for the explanation, the issue is indeed much more complicated than just removing the check, I didn't realise it. This just shows how complicated the builder is and that I should dig deeper with issues like these... I'll close this PR.

@mslourens mslourens closed this Feb 24, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Feb 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty screens tab not visible after selecting layouts tab
4 participants