Layer violations in Structure101 #140874
Comments
|
In an attempt to simulate tests being separate from production code and see if production code uses test code or not, I made use of Model Transformations in Structure101:
The need for many rules comes from the multiple naming conventions present in the code. There are cycles from vs into test even with the transformations above:
Do we want to have sub-layering inside platform? Note that in platform there is also a small tangle between diagnostics and launch: and a much bigger tangle:
Workbench also shows a tangle:
|
|
On @sglebs's prompting I took a look at the code in S101 Studio. I notice that the workbench folder contains an underlying file tangle of 749 files. I think you'd need to break that up a bit in order to layerize the workbench sub-folders. Removing just a couple of outgoing dependencies from workbench.web.main.ts would (if appropriate) break the big file tangle into much smaller ones (max 42 classes), which should make restructuring to a layered structure much easier. I'd be happy to contribute if this is important to the team - let me know. |
|
Some feedback:
|
bpasero
added
the
under-discussion
|
Regarding test code: IMHO tests should sit on top of code that enables testing, but not the other way around. In my second comment above I have used model transformations to simulate tests being in a separate root "namespace" ( you will notice that I added a synthetic /[test] folder/package at the top). It may be worth tweaking the rules I used, maybe I moved too much into /[test] (I would need a deeper knowledge of the codebase at this point). Anyway, here are the rules I used to move test code to an upper layer "/[test]": and here are the violations that remain according to the above logical layering:
Probably some entries should not have been moved to the upper layer "/[test]" in my model transformations - this can be easily changed once it is confirmed/determined by an insider engineer/architect. Another approach is to simply skip test from the analysis. I avoid this because I have seen my fair share of production code using test code in previous lives, so I like to see a clean layer structure where "/tests" uses code underneath but not the other way around. Model transformations are great because they allow this logical view & enforcement without imposing a strict folder structure on the teams. Congrats on the great work, let me know if I can be of any further assistance. I can easily run the analysis again, tweak the model transformations, etc. Cheers! |
|
I'm excited to see the power of Structure101 now being harnessed for TS/JS and applied to this codebase, where architecture is held in such high regard. |
|
I don't understand the layer violation from If we wanted strict layers for our API, we would have to move the API pieces into each respective component (e.g. |
|
@bpasero it may be the case that the existing "packages"(folders) are not true components and/or layers, so circularity among them is not really a violation of the Acyclic Dependency Principle among components. In this case a logical layer containing more than one "physical" package/folder can be created as well (at least in Structure101 this is easy), so you might have circularity inside but the thing is supposed to be reused as a whole, so no big problem. Other characteristics of the layers/components should be checked (Reuse-release Equivalence Principle, etc - more details here) to decide on the proper layering. Alternatively you can create a logical layer/component with parts from separate physical folders. For example, one can create an [api] folder/package/layer as a logical thing and enforce rules for it, despite having parts spread out "physically" across folders. The possibilities are many, but I do not know how to enforce this with eslint rules. GOTCHA: Note that bundling packages/components to form a bigger logical package may get you off the hook regarding Tangle at higher levels, but it may produce a bigger component, which increases what Structure101 calls Fat. Here is the current situation of Fat Versus Tangle in VSCode according to the analysis I did:
Trading Tangle for Fat won't get you any closer to the origin (0,0) in most cases. Feel free to contact me privately if you want to bounce some ideas or just want to see how things are in the tool. If I can be of any help to the project, just let me know. |
|
Thank you for the analysis! Unfortunately I was unsuccessful in running Structure101 on our sources. On macOS, I could only find version 1.1.0 of the plugin, which after a
In parallel with this issue, prompted by #140857, I spent some time to improve our rules and our ESLint plugin that validates our imports. I pushed those improvements via 62b0d5c . I have also identified about 9 layer breakers, 2 of which in test files (marked in our sources with |
|
Going through problems reported in this issue (using ✅ to mark problems that are tracked in other issues):
@sglebs I'd love to get Structure101 running. Do you have any insights into how to overcome the Out-Of-Memory error that I'm running into? |
|
@alexdima did you point s101 at the sources in vscode/src ? Or did you point it at the root dir "vscode"? You will see that my documented steps say to point at vscode/src and the tsconfig file vscode/src/tsconfig.json. I will investigate the Out of Memory Error, perhaps pointing it at the root folder vscode (instead of vscode/src like I did). Thanks! |
|
@alexdima 1.1.0 does not support the tsconfig file of vscode, make sure you run 1.1.3. It should now be available there now (I think it had not been uploaded to the s101 site yet by the time you tried). In my case I actually deleted my ~/structure101g/flavors/net.betterdeveloper.javascript.imports_1.1.0 to make sure I would not make a mistake of running it. Version 1.1.3 supports the tsconfig format present in vscode. Also, 1.1.0 was quite slow. Try 1.1.3 pointing at vscode/src and let me know how it goes please. |
I can look into that if you report an issue. |
|
@alexdima I adjusted Model Transformation according to your insider info that "the Model Transformations are slightly incorrect because we have a feature called testing, so /testing/ or */*Testing.ts should not be mapped to a separate [test] namespace" and now I only see 2 violations there: The adjusted Model Transformation rules are: (The last 2 rules are necessary for the cases where more than 1 rule is applied to the same element - a sort of clean-up at the end) |
🤦 That was the problem! Sorry about that, I'm now able to run the analysis. Thank you! |
|
@alexdima Glad to hear you got it going! Over here, even pointing at the root folder (incorrectly) I can't cause Out of Memory. I have a 2016 MacBook Pro, 16GB RAM here (old machine by today's standards). Do you mind sharing detailed info on the hardware that caused OoM with plugin version 1.1.3 pointing at the root folder? Is it an M1 perhaps? Or maybe an even older Mac? Thanks! |
|
@sglebs I am trying on a MacBook Pro 2019, 32GB RAM. There must have been some unlucky timing, maybe you were releasing I then switched to a Windows 11 VM with 8GB, where I could see But after correcting and pointing to |
|
@alexdima regarding your observation "the tangle is very likely caused by implementations of services depending on other services. For example, we have a log service implementation which uses the file service to write to disk, or we use the log service for logging in the implementation of the environment service." it is important to note that services using services is ok if the dependency graph is acyclic (ADP - Acyclic Dependency Principle). You will only get a tangle in the tool if A uses B and B uses A (directly or indirectly). The question to ask is: should they have these cyclic dependencies? Is this desirable? Sometimes a simple service injection at runtime for the "feedback dependency" will solve it. Or maybe the feedback dependency is not supposed to be there at all. You get the idea. (In concrete terms, I mean if the Environment Service needs the Logging Service and vice-versa, just decide who the "bottom-most" one is and inject the "above" one as a runtime dependency instead of compile-time dependency, as this will make the bottom one reusable on its own due to no tangle). Just some thoughts, I am not an expert on the inner workings of the current code. Just my 2 cents. Congrats on the work! |
|
@alexdima FYI the plugin generates FAT and SIZE numbers for files. FAT comes from the total cyclomatic complexity of the file. Size comes from the lines of code (LOC). S101 also computes FAT for modules, and then the final FAT % shown in that nice green/orange 2D graph. You may want to have FAT under control as well. @chedgers do you care to talk a bit about Fat as well? Share some wisdom applied here? Thanks! |
|
"Fat" is too much complexity in one place for a developer to easily understand. The s101 standard is to use cyclomatic complexity for functions since "number of paths" is a better measure of understandablilty (than say just LoC). The same principle is used at higher levels of composition. So at the file level, we count the number of function-to-function dependencies (rather than say just the number of functions). At the folder level it's the number of file-to-file dependencies. Containment is a kind of coupling, so a Fat file can be dependent on/from an excessive number of other files - compared to if it was split into several simpler files, each with a fewer dependencies. In this way excessive Fat makes a codebase harder to restructure (to e.g. remove tangles, fix layering). Often splitting a Fat file is a useful precursor to resolving layering problems. The reduced granularity of dependency making it easier to relocate a subset of files/functions and remove feedback between folders... Limiting Fat as the code develops keeps the codebase more understandable and flexible. Actually given the JS/TS flavor stops at the file level, I was pleasantly surprised to see a measure of Fat for that @sglebs - I presume that's the sum of the CC for all functions in the file? |
|
That is correct, @chedgers . The plugin generates Fat for a file as being the sum of the CC for all code in the file. It also generates the Size property as being total LOC of the file. |
|
Is there a way to configure certain layers to be the same layer? These two:
Are a somewhat expected tangle and it is probably unrealistic to untangle it but it makes it harder to see other tangles. |
|
That's what transformations do. It might work if you map |
|
Yes transformations is a good way to do that. You could also use "actions" to do the same thing... right-click/unwrap on the |
|
I am curious: was our JS/TS plugin for Structure101 useful after all? Have the tangles been mastered/eliminated? Were the ESLint rules patched to be more robust? I ended up mentioning this incident at https://www.linkedin.com/pulse/javascripttypescript-code-does-have-mess-marcio-marchini/ |
In the process of testing our plugin/flavour for Structure101/g which parses JS/TS projects, we identified a few problems with the layering in VSCode 08b9273. In a private channel, @egamma asked us to enter this issue.
Steps to Reproduce:
When the conversion finishes, you will get some layer violations:
I will enable/explain Model Transformations and enter a separate comment for that mode (extracting tests out into a separate structure).
The text was updated successfully, but these errors were encountered: