You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Make sure the preference order is the right one. This could partial fix if @dangoor would want to change the adding to scope as per order of call (as opposed to order of resolving promises which is how it is done now).
@busykai Your solution to the problem seems correct. I hadn't made the connection that the session MemoryStorage-based Scope would be added first.
The one concern I have is the possibility that your extension could try to set a pref before the session scope has been added. Does that seem like a possibility?
That kind of problem in your extension would not be something that we'd need to address in sprint 36, I think, but it's good to consider if we need to address it in 37. Maybe we add an event for when the preference manager is completely configured. Thinking about it, that would only be a couple more lines on top of this fix, if it seemed necessary.
@dangoor I do not think it's likely. However, I do agree, that it's not guaranteed by how code is organized, but by the amount of IO between the two events (all the languages are loaded + all the extensions from default and dev). And it's not nice.
I think it could be done through an additional event as you describe or it could also be done as part of the init process (in document ready).
Currently init is done as sequence of LanguageManager => ExtensionsManager => ProjectManager => app ready, it could be easily done as (PreferencesManager and LanguageManager) => ExtensionsManager => ProjectManager => app ready if PreferencesManager exposed a promise (in a similar way LanguageManagerdoes it). That would guarantee that it will be initialized by the time extensions are loaded.
Aside from that, I was thinking organizing a queue in PreferencesSystem so that scopes would be loaded in call order and as their storage is loaded, but it does seem too invasive and there will not be too many "system" scopes to chain them "manually" as this PR does. Those extensions who install their scopes should take care of their proper loading and handling themselves.
@busykai Making sure that the base scopes exist before initializing extensions seems like a good idea.
Another way to approach this would be to have some kind of general name for the path-based scopes so that the session scope can explicitly add itself before the path-based scopes. If we did that, there may not be a need for a full queue for scopes.
I'll go ahead and merge this branch into release and master.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
None yet
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make sure the preference order is the right one. This could partial fix if @dangoor would want to change the adding to scope as per order of call (as opposed to order of resolving promises which is how it is done now).