Size of dependency graph

wincent commented 4 years ago

Just planting this one as a seed for discussion and thought, although it may not lead to any action. The question:

What can we (or should we) do about the size of the Clay dependency graph?

The Clay dependency graph is very large.

The yarn.lock has 23k lines.

yarn audit reports:

13427 vulnerabilities found - Packages audited: 960275
Severity: 11 Low | 11 Moderate | 13405 High

(Note that it is obviously some double-counting there due to some kind of Yarn bug; there are 80k files in the node_modules directory and it must be counting the same packages repeatedly.)

Running yarn outdated 108 outdated dependencies:

Package                          Current      Wanted  Latest     Workspace                Package Type           URL                                                                                          
@babel/cli                       7.4.4        7.8.4   7.8.4                               devDependencies        https://babeljs.io/                                                                          
@babel/core                      7.4.5        7.8.4   7.8.4                               devDependencies        https://babeljs.io/                                                                          
... 104 lines omitted ...
yeoman-generator                 0.19.2       0.19.2  4.5.0      generator-clay-component dependencies           http://yeoman.io                                                                             
yosay                            1.2.1        1.2.1   2.0.2      generator-clay-component dependencies           http://yeoman.io

Full output in this gist

Screenshot showing the output in all of its glorious red/yellow:

Obviously, all of these dependencies bring some value, otherwise we wouldn't have added them. Most — but not all — of them are devDependencies. But I wanted to bring up the size of the graph because these dependencies are not free:

The larger the graph, the higher the cost of keeping it up to date (something sadly necessitated by security problems, which mean that we don't upgrade because we need access to new features or bug fixes, but because we're forced to by an audit).
Of the 108 outdated dependencies, 50 of them are semver-breaking major updates. The cost of updating these is hard to evaluate but probably pretty large. At some point, the cost may start to exceed the associated benefits.

Just to put things in perspective, liferay-portal itself "only" has an 18k-line lockfile (about 82% of the size of Clay's).

As I said above, I don't know if any specific action needs to or will be taken as the result of this ticket, but I just wanted to raise awareness about the cost-benefit trade-off involved in maintaining this dependency graph. We might want to check to see if there are any non-essential dependencies, or dependencies that aren't pulling their weight. Or we might merely keep this discussion in mind the next time a PR proposes to add another dependency to the graph.

Sighed, your grumpy, dependency-allergic curmudgeon, @wincent.

clint-eastwood

matuzalemsteles commented 4 years ago

Oh I'm surprised that our lockfile is bigger than Portal 😱. But looking at it quickly it seems that we have a lot of outdated dependencies concentrated in the @clay/css package, alias it doesn't work with Node 12, so maybe we need to update the dependencies or modify the build stack to modernize, we would probably be able to decrease this graph of dependencies but I'm not sure how expensive it would be.

bryceosterhaus commented 4 years ago

Thanks for opening this @wincent, this is a great issue to investigate on a rainy day. I would suspect that the clayui.com and @clayui/css package are two main culprits. I'll look into this in the next few weeks.

pat270 commented 4 years ago

To me @clayui/css test site is important, maybe I can keep a local copy for development then we can be done with this issue.

jbalsas commented 4 years ago

Hey @pat270! If the test site is important, it's important for everyone :)

At the same time, it's also important for everyone that the project is light, easy to build and contribute, so we need to find ways to bridge both worlds.

@bryceosterhaus, since we're already using Gatsby to generate the new site... why don't we just migrate the test site to Gatsby? That might help reduce dependencies while keeping a tool like the test site in a more orderly fashion... Additionally, coming up with a nicer organization in the test site could lead to better synergies and allow us to, for example, import samples directly from there rather than having to rewrite them in the clayui.com project... wdt? 🤔

bryceosterhaus commented 4 years ago

@jbalsas When you say test site, are you referring to storybook?

jbalsas commented 4 years ago

No, @pat270’s test site... the thing is not clayui.com nor storybook

bryceosterhaus commented 4 years ago

ah gotcha. i didnt realize that was in the repo. @pat270 how do you typically run it?

pat270 commented 4 years ago

@bryceosterhaus packages/clay-css/ then gulp serve

bryceosterhaus commented 4 years ago

Okay so I spent some time trying to get a POC for moving the clay-css test site to gatsby so that we can remove a bunch of dependencies. I tried to preserve the structure of our content as much as possible, meaning we needed to configure gatsby to dynamically create pages based on html files with frontmatter.

Overall, the goal was to utilize gatsby since it was an existing dependency for clayui.com, and then we could remove a large portion of our devDependencies that are focused on supporting the test site.

I was able to get the site running and dynamically generate the pages we had previously, but I ran into issues with how it was transforming the html. One big area of concern was that I was unable to get our <code> examples to properly render with html content escaped inside. I was utilizing gatsby-transformer-rehype to handle this, but just couldn't quite get it to the place I wanted. I was also using html-frontmatter to preprocess the html and get the frontmatter information.

I'm not sure if anyone has any other ideas or if this is even worth the time pursuing any more.

Here is a demo of the site with gatsby. I'm sure a handful of interactions are broken, I haven't added the js back in yet. Ths focus was on dynamically generating pages. Here is the code

So overall question: is it worth spending more time migrating to gatsby? Or do we just let it be and put up with additional dependencies since its working fine as is.

wincent commented 4 years ago

So overall question: is it worth spending more time migrating to gatsby? Or do we just let it be and put up with additional dependencies since its working fine as is.

The answer to that probably depends on whether you have more important things to do, but I don't actually know all the things you have on your plate right now so I don't have much to add here. 😬

jbalsas commented 4 years ago

So overall question: is it worth spending more time migrating to gatsby? Or do we just let it be and put up with additional dependencies since its working fine as is.

I honestly think we should kill that. Ideally, we'd find better development workflows than having to check demos on a huge "test" site. It's also often used as a deep-web documentation which really bothers me, and feels like another reason we should get rid of that and focus on our site.

My feeling is that the effort of migrating it AS IS is too big. I'd suggest approaching this from a different angle and make a full assessment of the situation

What do we need the test site for?
What would be a good way to achieve those requirements?

Based on the answers to those questions, we can plan and prioritize accordingly.

edalgrin commented 4 years ago

Please don't kill it before we get all the information it holds https://github.com/liferay/clay/issues/2734 (🤔 the sentence seems to come from a mafia movie 😆)

pat270 commented 4 years ago

I agree we shouldn't port the test site as is. I think we can develop using clayui.com with the current content as long as we fix these issues:

1) Right now we use node_modules/@clayui/css files for clayui.com, which isn't very useful during development. If I want to use clayui.com to test changes in @clayui/css I need to update package.json to point to my local @clayui/css, update clay css, and restart the server everytime I want to view an update.

2) Need to be able to switch between base theme and atlas theme.

@eduardoallegrini It should be ok to kill it right now. It doesn't take me too long to upload archives to my github pages. The information will still be available on the deep-web documentation :laughing: mentioned by @jbalsas.

bryceosterhaus commented 4 years ago

@pat270 what I'll do is try and get clayui.com in a usable spot for your development and also keep your old site around for now. Once we are able to provide the same value on clayui.com, we can then kill the test site.

Once I address your two points, just try to make an effort to use clayui.com, and then any time you find yourself going back to the test site, just ping me and we can create and issue and I'll work on a solution so that clayui.com is viable for you.

Thant work?

pat270 commented 4 years ago

@bryceosterhaus sounds good. Thinking about it more, even having only 1) working would suffice.

liferay / clay

Size of dependency graph #2891