Skip to content

Conversation

@indykoning
Copy link
Member

Added a wait for config to be available, in case of issues a max of 1s is used

@indykoning indykoning requested a review from royduin as a code owner December 11, 2025 11:05
@royduin royduin requested a review from Jade-GG December 11, 2025 14:11
@Jade-GG
Copy link
Collaborator

Jade-GG commented Dec 11, 2025

  1. Waiting for 50ms a total of 20 times doesn't feel like the cleanest approach. Would it be cleaner to listen to an event, e.g. by putting an onload on the script tag, or even fire a custom event inside of the js file by adding to config.blade.php? Could even also set a window variable like window.configLoaded so you don't have to specifically check for config.store.

  2. Should we have some sort of basic fallback data in case the config doesn't load? Perhaps at least making the notify function work, as that seems to be the most common error.

@indykoning
Copy link
Member Author

  1. Waiting for 50ms a total of 20 times doesn't feel like the cleanest approach. Would it be cleaner to listen to an event, e.g. by putting an onload on the script tag, or even fire a custom event inside of the js file by adding to config.blade.php? Could even also set a window variable like window.configLoaded so you don't have to specifically check for config.store.

We could fire a custom event from inside the JS file and await for that if config is missing, though i am in favor of still keeping some kind of timeout to prevent Vue from booting at all if config cannot load for whatever reason.

  1. Should we have some sort of basic fallback data in case the config doesn't load? Perhaps at least making the notify function work, as that seems to be the most common error.

While some fallback data would be nice the biggest issues are within templates (e.g. graphql queries) which aren't reactive, thus you will always be stuck with the fallback values.
The Notify function does now work, it is dependent on a Vue component. By extension Vue loading, which we check now.


A future enhancement could be utilising a Store for the notifications, since (with turbolinks) it survives page navigations which we currently use different workarounds for.

@royduin royduin merged commit 0e76afd into master Dec 12, 2025
12 of 15 checks passed
@royduin royduin deleted the bugfix/wait-for-config branch December 12, 2025 12:31
@royduin
Copy link
Member

royduin commented Dec 12, 2025

Follow-up: RAP-1713

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants