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

Resolve circular dependencies #375

Closed
pjasiun opened this Issue Apr 22, 2016 · 5 comments

Comments

Projects
None yet
4 participants
@pjasiun
Copy link
Member

commented Apr 22, 2016

@pjasiun pjasiun added the intro label Apr 29, 2016

@pjasiun pjasiun referenced this issue May 12, 2016

Closed

Stabilize the API #409

2 of 17 tasks complete

@pjasiun pjasiun modified the milestone: 0.2.0 Jun 17, 2016

@scofalik scofalik self-assigned this Jul 20, 2016

@scofalik

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2016

Below post assumes that #480 is done

image

(here's a link to the sketch if you want to play with it: https://sketchboard.me/Tz1FbT5aiDkv)

This is a sketch that I've made that shows up imports between classes in model. I hope I haven't missed anything. View is very similar. In model there is also modelWriter but it uses a lot of classes and is not used by anthing, so it makes up only unimportant mess. The sketch does not consider utilities but utilities repo is not depended on engine, so there can't be any hidden dependencies there.

Here are some conclusions that can be drawn from sketch:

  • still too many imports. The graph ended up much more complicated than I thought, but it's better than it could/was. A lot of those imports are just for instanceof purposes, and the rest is mostly for extending. DocumentFragment needs Element and Text only for it's fromJSON method, in same manner Element needs Text. LiveSelection uses a lot of stuff too, maybe it can be optimized.
  • lines go only down or sideways (in fact each time a line go sideways it could be another top-down layer but first it would make the sketch very tall, and second, this way classes are organized in a bit more logical way). This means no circular dependencies if there are no circular dependencies in one layer.
  • I am glad that I was able to "cut out" TextProxy of Text, Element, Node section -- right now it is more of a utility class for TreeWalker.
  • there are a few logical layers:
    • Node and NodeList are base for all the structure
    • model tree is based upon them, using Text, Element and RootElement
    • DocumentFragment uses all of them and is "highest" point in the structure as far as model tree is concerned
    • then there is another layer - traversing model tree - opened up by Position
    • TreeWalker uses it's utility class TextProxy and Position to bring ability to walk the model tree
    • then there is Range that uses Position and can be iterated in different ways thanks to TextProxy
    • we have LiveRange and LivePosition which are kind of Range and Position
    • Selection and LiveSelection is build on top of this API

madge tool also shows that the artchitecture is good. The only circular dependencies that are left are in operations and deltas when they create each other when reversed:

ckeditor5-engine (t/165)
$ madge --format es6 --circular src/
model/operation/removeoperation -> model/operation/reinsertoperation
model/delta/mergedelta -> model/delta/splitdelta
model/delta/unwrapdelta -> model/delta/wrapdelta

For now we got rid off circular deps in model tree structure and it's API. The question is whether I should work more to make this graph even more clear or is it okay right now? TBH it will be hard to make it any better and I fear that many solutions would just come down to hide the dependencies in new classes/tools.

@scofalik

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2016

Of course this only shows import, which is just half of truth. There are other dependencies, i.e. TextProxy#textNode property is Text instance, but this is different type of graph. If I had to draw this one it would be a lot more complicated, I fear.

@scofalik

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2016

I've updated the scheme to make it more UML correct and clean:

image

(still available at https://sketchboard.me/Tz1FbT5aiDkv)

I've added relations if a class A is a property in class B but is not created by it and does not compose class B. It mostly shows "parent-children" association. It's denoted by arrow with black full point. You can see how TextProxy is associated with Text and Node is associated with Element. Those classes have references to their "parent" classes (parent in structural meaning, not inheritance meaning). I've skept those arrows if a class composes another class (i.e. RootElement has reference to Document but their association is already denoted by composition arrow).

I've also changed some arrows. Now you can see when class A composes class B (i.e. Document is composed by many RootElements).
Dashed arrow means that class A imports class B but does not instantiate it nor has it as a property. Those arrows means "instance of" imports.
Solid arrows with full white point means inheritance.
Solid arrows with not-full point means other relations. Mostly creating instances of given class but not using them as property or importing for "instance of" purposes but also having instance of given class as property.

Below is the same scheme without "property" relationships:

image

@scofalik

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2016

I've updated first image in post above. Also, I think, that we are fine not caring about property-dependency as long as we do not import given file (so second image is more important, IMHO).

@pjasiun

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2016

Fixed by #537.

@pjasiun pjasiun closed this Jul 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.