-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(layered-storage): add a new storage for options #71
Draft
Thomaash
wants to merge
54
commits into
master
Choose a base branch
from
layered-storage
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains 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
This is intended to replace the prototype plus hacks insanity used at the moment. However this is still WIP. There is no documentations yet and more testing is necessary. TODO: - Add off. - Write docs. - Probably some other things too. In a quick test I was able to resolve the issue discussed in visjs/vis-network#178 and visjs/vis-network#213 with just a few lines of code. Which is much better than the massive mess of weird hacks that doesn't work reliably anyway. Putting this to use will be a lot of work but fortunately it should be possible to do it in parts. I would first use this in LayoutEngine and EdgesHandler to resolve the forementioned issues and then probably one module at the time. Features: - Encapsulates options merging. - Explicit layer/segment/key structure instead of prototype chains. - Observable. - Overrides. * - Type safety in TypeScript. * Hierarchical layout is incompatible with smooth edges and has to disable them. Overrides combined with observing easily and elegently solve that. See the forementioned issues for current state.
About half should be done by now.
It makes more sense given what it actually does.
Caching up front would be too complex to implement properly without rebuilding the whole cache due to every change.
The sorting fails and TS already prohibits them. There's no indisputable way of sorting nonnumbers anyway.
This is to simplify debugging. It logs the content of the storage to the console using collased groups to denote the structure of the storage.
Previously the structures were kept even if all values were deleted.
BREAKING CHANGE: The on methods were removed from Layered Storage. This was unbearably slow. Most of the time was actually spent handling events. Making it faster without reducing it's functionality to the same level as Emitter seems impossible to me. Since we already use the Emitter there's no point in having such thing.
Prior to this all cache for given key was cleaned on set or delete. However this is only necessary for monolithic segment. Other segments can't affect anything but themselves so it's okay to keep the other segments cached. This greatly improves set and delete performance.
This greatly improves performance on mostly static data as it avoids repeated traversing through the data just to find nothing over and over again.
Before this all segments were iterated over and then checked for existence. This is faster as it iterates only over existing cache without checking for each segment's cache existence and also less code. Also includes a test for possible regression that was unchecked before.
These are used to expand short hand values to their full forms.
BREAKING CHANGE: addValidators no longer exists Appending validators to already existing one is most likely an error. At least in Vis Network I see no reason to do it. Therefore it makes more sense to throw in such a case.
BREAKING CHANGE: Some types were renamed. It has the same format as the various .entries() methods so it makes more sense to call it entries.
Those are a few micro optimalizations. However since this is executed over and over again it adds up to noticable improvement.
BREAKING CHANGE: Most stuff was changed. This is a really big overhaul that greatly reduces the API and the codebase. This should be much easier to maintain and also more performant when used at the expense of removing support for some niche features.
Thomaash
force-pushed
the
layered-storage
branch
from
January 26, 2020 21:45
e25f252
to
cc0f866
Compare
This will make integrating this into existing codebase much easier. We'll be able to mostly contain the changes into .setOptions() methods.
Prior to this only the commit would fail.
This can be used to implement groups from Vis Network. It actually allows for multiple groups per single node which is already a wanted feature that would be extremely difficult to implement using the (anti)pattern that's currently in Vis Network for options.
This also includes a tweak to the way validating is done.
BREAKING CHANGE: The types are different, though the code is pretty much the same on the outside. This allows for typesafe expanding of values or transforming them (expanders allow both at the same time).
BREAKING CHANGE: This completely prevents null from being used as a value. We could change it to a symbol if this turns out to be a problem.
Since it's not possible to use custom tsconfig.json it only works with what the authors configured it to work with. There's an issue on their repo tracking this. It doesn't work with layered storage because it uses iterables other than arrays and that's not configured in check-dts.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is intended to replace the prototype plus hacks insanity used at the moment. However this is still WIP. There is no documentations yet and more testing is necessary.
TODO:
In a quick test I was able to resolve the issue discussed in visjs/vis-network#178 and visjs/vis-network#213 with just a few lines of code. Which is much better than the massive mess of weird hacks that doesn't work reliably anyway. *
Putting this to use will be a lot of work but fortunately it should be possible to do it in parts. I would first use this in LayoutEngine and EdgesHandler to resolve the forementioned issues and then probably one module at the time.
Features:
Observable.*** Hierarchical layout is incompatible with smooth edges and has to disable them. Overrides combined with observing easily and elegently solve that. See the forementioned issues for current state.
** This worked very nicely but it slowed exponentially with growing number of items rendering this feature pretty much unusable.
*** Now we sometimes ignore, sometimes log and sometimes throw. In other words it's an unpredictable mess. In LS a single function handles feedback for everything.