- Issue created by @balintbrews
- πΊπΈUnited States effulgentsia
balintbrews β credited effulgentsia β .
- πΊπΈUnited States effulgentsia
This is still an important issue to get done, but we started a shorter-than-usual sprint following DrupalCon, and are choosing to prioritize some other issues ahead of this one. We'll pick it up again in an upcoming sprint.
- π³π±Netherlands balintbrews Amsterdam, NL
Changes by this issue are going to significantly effect the code for saving JS component data. I would like to take this opportunity and improve the auto-save logic. Here is my high-level plan.
Make auto-save more robust
Auto-save currently works by subscribing to any changes in the code editor Redux slice. The complexity of this is that there are changes that will occur upon loading the data for the first time, and we need to ensure that those are not triggering an auto-save.
The way this is currently implemented is fragile: The implementation makes the assumption that changes that occur first are from data loading, and whatever changes come after are good to auto-save. Certain race conditions can easily break this β and they have in test environments.
I would like to update that logic in a way where the actions dispatched to update the code editor can express whether auto-saving is desired. So instead of the subscriber logic trying to figure out when auto-save is appropriate, we let the individual actions tell when it is not. By default we should assume auto-save is desired, as that is the more common case.
Fix auto-save UX issues
The current auto-save implementation has a big problem. Data can be lost if users navigate away from the code editor route before saving the data has been completed. This can sometimes take up to seconds, especially in slower environments. π Auto-save changes from code editor get lost if you navigate out too quickly Active tracks this problem.
To mitigate this, we can delay navigation using the
useBlocker
hook of React Router in the following cases:- Auto-save is pending;
- JS/CSS compilation is in progress.
Whenever users try to navigate away, we should
- display a toaster informing the user that saving/compilation is happening;
- disable code editor input;
- disable editing props and slots.
Once the JS/CSS compilation is finished, and auto-save is done, we'll automatically proceed with the navigation.
An important change that is required for this, which will also help making auto-save more robust in general, is that auto-save should not be subscribed to changes in the compiled JS or CSS. Instead, finishing the compilation should dispatch the auto-save action. This way we ensure that an auto-save always explicitly follows a compilation. Navigating away can be sufficiently delayed, and compiling the preview for the first time will not trigger an auto-save. (Preview is always compiled, never read from the saved data.)
- Merge request !974#3516390: Compile Tailwind CSS globally for code components β (Merged) created by Unnamed author
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
balintbrews β credited larowlan β .
- π³π±Netherlands balintbrews Amsterdam, NL
Thanks, @larowlan, for the sanity check on my tests, and pointing out inefficiencies with the new CI job.
- π³π±Netherlands balintbrews Amsterdam, NL
This is ready for a first round of review. I haven't done the "Fix auto-save UX issues" part from #7, but I added an
isCompiling
andisSaving
status to the Redux slice. I'll move the rest of that work back to π Auto-save changes from code editor get lost if you navigate out too quickly Active , this MR is already enormous.I recommend starting the review from
useCodeEditor.ts
and move towards the hooks used in there. To understand what's happening, there is a flowchart in the MR. I refactored codeEditorSlice.ts, which produced changes in many files: I added comments to all of those with a π’ emoji to help skipping over those.The MR also introduces Vitest for component/unit tests. I really wanted to have these lower level tests for the updated functionality, and Cypress components tests are not suitable for mocking/stubbing ES modules.
- First commit to issue fork.
- π³π±Netherlands balintbrews Amsterdam, NL
Note that we'll need π Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI Active for a full experience. Thanks to Wim for pointing this out.
-
balintbrews β
committed ce566348 on 0.x
Issue #3516390 by balintbrews, jessebaker, effulgentsia, wim leers,...
-
balintbrews β
committed ce566348 on 0.x
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Reopened π Auto-save changes from code editor get lost if you navigate out too quickly Active per #12 π