smikula / good-fences

Code boundary management for TypeScript projects
MIT License
184 stars 18 forks source link

Other projects in a Rush monorepo are not seen as external. #75

Open BrianMar opened 5 years ago

BrianMar commented 5 years ago

I'm attempting to add good-fences to a project in a monorepo using Rush. When I import a file from another project in the repo, I get import errors for those projects.

Proposal: Instead of just checking if the import is in the "node_modules" folder, good-fences should flag anything outside of the "rootDir" as external. If I'm only checking for fences under the root, then anything outside that root should be considered external.

smikula commented 5 years ago

I haven't worked in a repo that used Rush, so I may not have the best perspective on this, but I'm wondering: why not just run good-fences across the whole repo? That's more in line with how I expected good-fences to be used, and at that point all those imports really are internal imports.

Your ask is pretty reasonable, but I'd like to understand why the above isn't a good fit for you. Is it a matter of you only own some subset of the repo and you don't want to impose good-fences on everybody?

My hesitation is that it's a little weird for a fence to act differently depending on where you run the tool from. If we go with your suggestion then it will work fine for your individual project, but if you ever want to go up a level and run it against the whole repo then all your dependency/import rules won't mean what you intended them to mean anymore.

BrianMar commented 5 years ago

Good questions, Scott.

The point of a Rush repo is to avoid having to maintain one repo per project but still provide the same kind of isolation that separate npm projects and repos would have. The linkage from one project to another is done using npm dependencies, and it is bad practice in a Rush repo to import outside of your project.

Internally, Rush takes all of the package.json dependencies and dev dependencies from all of the projects and then merges them together and does a single install to a shared folder. Then, instead of having one download per project reference, Rush symlinks the dependencies in the node_modules folder. This is true for both dependencies from npm servers and from those that are also projects inside the tree. So if have a rush repo with packages a and b, and b depends on a, then b’s package.json will have a “dependencies” entry for a. For all intents and purposes, a is a separate package that b should have no internal knowledge of. So have the fences for b refer to any tags in a is a violation of this intent. Every “dependency” in the repo is treated as an external dependency by Rush.

The problem is that, when tools (like good-fences) query for the path to a/node_modules/b, windows returns the absolute path instead of the symlink. So instead you get a link to /a/lib, which is where the output from a is stored. This makes sense, from a build process point of view, since the symlinking avoids file copies and the output of a is available to all downstream dependencies at no cost.

Aside from breaking the intent of project isolation in Rush, monorepos also get so large that having tooling that is searching the entire repo is both functional and mentally out of scope. I tried running good-fences on our entire repo, with only one project of the 28 having any fences files (and only in 3 of 10 top-level folder). Yes, that’s the kind of spaghetti I’m trying to untangle. We have 10 top level folders, with multiple sub-folder levels at places, and we have some code that links circularly around the tree and only seems to work due to luck. Anyway, when I run good-fences, node runs out of heap space. And we’re in what I consider a pretty small monorepo. Before we pulled out our projects, we were in a larger repo. Combining our 28 with their current 68 projects, we’re close to 100 projects.

What we’re finding is that these Rush monorepos also tend to accumulate projects that are tangentially related but aren’t necessarily coupled. For example, we have office-online-ui, which is a bunch of projects that share some core code but then end up producing UI elements for word, powerpoint, xl, etc. The actual shared footprint is small. But having the same set of tags have to be kept consistent between all of the teams involved seems very unwieldy.

There is also the case of not wanting to impose good-fences on the whole repo. Of the 28 projects in my repo, I work on maybe 5 of them regularly. My use case for good-fences is to help prevent my team from continuing to make the same bad coding decisions with regarding to imports that we’ve been making for the past year. Other teams haven’t made those choices, or have separated out their code into smaller projects to prevent this problem. So unilaterally imposing good-fences on them would be a tax they won’t want to take. And I would agree with them. Right now I’m trying to layer our code into a form that matches the idealized form the design had in the early stages of the project, which has been subverted by the ease of importing from anywhere in a TypeScript project. The code is so riddled with incorrect imports that I can only currently apply good-fences to 3 of our 10 top-level projects, and I think I’ve got about 2 weeks of work just to get the 4th layer of fencing in place. So it’s not feasible to apply fences to the whole repo when we can barely apply them to a single project.

From the use case of Rush, I don’t think apply good-fences on the whole repo would be a good practice. It violates the pretense of rush treating each project as its own npm package within the monorepo. Also, with rush targeted at “monorepos with hundreds of projects” (rushjs.io), it doesn’t really scale to think about running good-fences at any level above the project. To integrate good-fences into Rush, you would create a rush “good-fences” “bulk command”, which are commands that run individually on the projects. This is the equivalent of how you run lint in Rush, where you lint each project individually and the concept of “lint” at the top level doesn’t exist, except is as much as it launches “lint” on all the projects. (More info at https://rushjs.io/pages/maintainer/custom_commands/).

Let me know if I can clarify further or answer any more questions.

Thanks, Brian

From: Scott Mikula notifications@github.com Sent: Wednesday, October 16, 2019 4:14 PM To: smikula/good-fences good-fences@noreply.github.com Cc: Brian Marshall Brian.Marshall@microsoft.com; Author author@noreply.github.com Subject: Re: [smikula/good-fences] Other projects in a Rush monorepo are not seen as external. (#75)

I haven't worked in a repo that used Rush, so I may not have the best perspective on this, but I'm wondering: why not just run good-fences across the whole repo? That's more in line with how I expected good-fences to be used, and at that point all those imports really are internal imports.

Your ask is pretty reasonable, but I'd like to understand why the above isn't a good fit for you. Is it a matter of you only own some subset of the repo and you don't want to impose good-fences on everybody?

My hesitation is that it's a little weird for a fence to act differently depending on where you run the tool from. If we go with your suggestion then it will work fine for your individual project, but if you ever want to go up a level and run it against the whole repo then all your dependency/import rules won't mean what you intended them to mean anymore.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsmikula%2Fgood-fences%2Fissues%2F75%3Femail_source%3Dnotifications%26email_token%3DACXCLUOTBKPTQZAV5UTTC2LQO6N4RA5CNFSM4JBEVSIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBOHACA%23issuecomment-542928904&data=02%7C01%7CBrian.Marshall%40microsoft.com%7Ca6f0a7fbd1944badfb2d08d7528e9139%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637068644595402579&sdata=PyseP2YtLcgyMlIpXKQiVg2GRU1wBqzlfpgGFGQmBFg%3D&reserved=0, or unsubscribehttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACXCLUJSN3REJS2L44IYCB3QO6N4RANCNFSM4JBEVSIA&data=02%7C01%7CBrian.Marshall%40microsoft.com%7Ca6f0a7fbd1944badfb2d08d7528e9139%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637068644595402579&sdata=A67fTpEELGwneAyFIY0tfxjEVYfMgIgo%2BxHXF1obVzI%3D&reserved=0.

smikula commented 5 years ago

Thanks for the detailed explanation! Your situation is exactly the kind of thing that I want good-fences to help in. I expect your type of monorepo isn't uncommon, and I see the sense in being able to configure good-fences on a project by project basis.

Short term I think it makes sense to go with your PR. (I have one quick comment that I'll make in the PR.) Longer term I'm going to look at whether I really need to differentiate between "imports" and "dependencies". I think I might be able to simplify things and reduce confusion at the same time.