Changing public folder in packages and enforce_privacy mechanism

[rafaelfranca]

Problem

One thing that is bothering me for a while is the way enforce_privacy is implemented in Packwerk. To fix privacy violations generated by the check, users need to move files that they intend to be used by other packages to a app/public inside their package.

The problem is that now this app/public folder is becoming a catch-all drawer containing models, controllers, services, jobs everything in the same folder. This defeats the organization of Rails applications, that most people love and cherish.

One of the tenant design decision of Packwerk is that it will respect and embrace the Rails philosophy, not go against that, and in this specific feature, it is going against an important tenant of what makes a Rails application a Rails application.

Proposals

1. Create a mechanism to export constants

It will consist in allowing developers to call a method to expose a constant to other packages. The Packwerk parser will look for exported constants, keep track of them and which package it is part of, and in a privacy check, will lookup in that collection to see if any violation is being made.

class SomePublicEntrypoint
end

export SomePublicEntrypoint

This will have the upside of making constants private by default, like proposed in #97.

This proposal will require that Packwerk define some kind of runtime component (which will be required by the packages and application).

1.1 Alternative implementation

Use comments instead of runtime methods (since we don't have any usage for the runtime aspect of that method). This will remove the need for the runtime component.

# @export SomePublicEntrypoint
class SomePublicEntrypoint
end

2. Move the public folder up one level

Moving the public folder up one level we will have it in the same level as app and we could have separation of concepts on it in the same way as app. This proposal will make public have the same organization as app with models, views, controllers, etc..

app/
  models/
  views/
  controllers/
  jobs/
public/
  models/
  controllers/
  jobs/

This will require that new load paths are added to every single package.

This is the solution I'm less keen to adopt since it goes against the work I'm doing to make all packages in our application to be just regular, with no custom configuration, Rails Engines, making harder for new packages to adopt the Packwerk structure, since it would not be plug-and-play anymore.

3. List all public files

Create a list of public files, in the same way that we have a list of deprecated references.

It could be automated with some kind of command to record a file as part of the public API of a component.

Like the Proposal 1, this also makes possible to mark all constant as private by default.

As this is a static list and we don't need to parse all Ruby files to find the list of public constants, it should be fast to execute.

4. Remove privacy checks

So far at Shopify, we didn't see much value in the privacy checks. Most of our packages have this option disabled and when we do have them enabled, people mostly fix this kind of violations mostly by moving files around, without improving the APIs, or, even worse, people just record the deprecations and forget about them.

Those ways to solve this violation are actually creating worse code, instead of improving it. The public folders are full of different concepts mixed together, and people are getting annoyed with "Packwerk failing on me with something I can't fix" and just avoiding the tool all together.

I know other users of Packwerk had more success with privacy checks, so this proposal is mostly "Remove privacy check to a Packwerk plugin". I know Gusto has interest on building a plugin system for Packwerk, so we could be using privacy checks as the first plugin to dog food the API.

Conclusion

I'm mostly inclined to go with Solution 1 or Solution 4, but I want to hear others opinion.

cc @alexevanczuk

[rafaelfranca]

cc @mclark as well

[alexevanczuk]

Thank you for this really thoughtful and interesting analysis! I'm collecting my thoughts and chatting more with my team, and I see a ton of opportunity here! I'll follow up with more thoughts soon, and I'd personally be excited to help solve this problem you're facing.

[shageman]

I am adding this as a placeholder comment for folks to add a 👍 if they use privacy protections today

[shageman]

I love option 2 for a variety of reasons, most importantly maybe that it elevates work on a public API in visibility.

The reason I have been ok with what we have today is that I actually believe that the investment in a public API shouldn't just be making certain constants public (for many packages). Rather, I believe there should be a deliberate investment in designing the API for the use-cases of other parts of the app.

[rafaelfranca]

I'm writing down Packwerk philosophy document now that we have a shared ownership to make it clear, but one of the tenants of Packwerk is that it should work out-of-the-box with vanilla Rails tools and don't make an Rails application deviate from what a vanilla Rails application looks like.

Registering custom load paths so Packwerk can work goes against that.

For the Proposal 2 to work, to create a package people will have to introduce a Rails::Engine and configure the that engine to register custom load paths, instead of only adding the Rails::Engine. This is significantly more work for the users of Packwerk to do in order to adopt it.

I listed that as an option mostly to exhaust the list of solutions I thought, and I can see how some people would prefer that, but since I don't see default Rails directory structure changing to that direction and because it causes more boilerplate for adoption, I think this is the proposal with less chance of being implemented.

About the goal of making an API public, what you described work well until you need to expose classes/modules to be inherited.

For example, if you have a base controller that all packages should use, where would that be placed? If it is in public, that cause the problem of "catch-all" drawer. If it is in app that violation will need to be recorded.

I agree with that goal though, but that isn't the reality we are seeing. A bunch of packages want to expose behavior to be reused. Of course you could create an object that encapsulate that behavior and use it in your controllers using composition, but that causes a lot of boilerplate in the components that uses that behavior. For controller specially, those boilerplates are complex and very prone to introduce usage errors, like it is the case of filter needed to be registered in every single component, sometimes with slightly different options that cause subtle bugs.

[bscofield]

I'd explicitly vote against 4 -- even if some changes made to eliminate a privacy violation are suboptimal (just moving files around, etc.), it still provides a mechanism to be more explicit about a package's public API, and we've found value in that even before intentionally redesigning that public API. It's a core part of Packwerk's value proposition for us.

We're currently working around the junk drawer problem by defining subpackages -- we'll have a top-level (e.g.) UserService pack, then one in user_service/app/models (with public_path set to .) that other packs can explicitly depend on if they need direct access to the models outside of the UserService's public API. This points to a possible 5th option, where Packwerk has better subpackage support -- the main change required being a way to inherit package membership. Maybe something like:

# UserService package.yml
enforce_privacy: true
enforce_dependencies: true
# don't need to specify that UserService depends on UserService models, b/c inheritance

# UserService app/models package.yml
public_path: .
enforce_privacy: true
enforce_dependencies: true
subpackage: true # activates inheritance behavior; inheritance would flow upwards until it hits a package.yml that _doesn't_ have subpackage: true
# don't need to specify that the models pack depends on other UserService subpackages, etc.

# BookService package.yml
enforce_privacy: true
enforce_dependencies: true
dependencies:
- packs/user_service/app/models # BookService depends only on UserService models, not on its public API endpoints or other subpackages

# ReviewService package.yml
enforce_privacy: true
enforce_dependencies: true
dependencies:
- packs/user_service # ReviewService depends on UserService and its subpackages

This doesn't completely address the issue, though, since it'd still be exposing all the models (in the case above) as part of the subpackage's public API 🤔

[bscofield]

Yeah, it's definitely not a perfect solution. I think my issues with 1 are that it requires you to edit every single file you want to make public (which could be annoying when migrating a large, highly-coupled application to Packwerk, but could also be automated away) and that it obscures the public API of a pack -- you won't know that a file is exposed unless you see a violation or you're looking at the actual file in question. Maybe combining it with 3 addresses the latter point (so you manually export constants, and then Packwerk or some other tool automatically generates a list of the constants that are public)?

[rafaelfranca]

I think we can combine 1 and 3, and that would also help with the migration between versions. That is what @iMacTia suggested below. We can even combine 1 with some documentation generation tool like yard or rdoc to generate a HTML page with all public interface of a component.

[iMacTia]

Yeah having a clear well documented public API is really important, and I'd happily part with the "public folder" concept if there was a nice automated way of generating a unified view/documentation 😃

[iMacTia]

We can even combine 1 with some documentation generation tool like yard or rdoc

And ideally Sorbet/RBS as well!

[exterm]

We can even combine 1 with some documentation generation tool like yard or rdoc to generate a HTML page with all public interface of a component.

As a matter of fact there was an artisanal one-off implementation of yard-generated component docs in shopify's core monolith, based on public folders, during my time there. It was never generalized to all packages though; I agree that would be great.

(Components are packages, but not all packages are components)

[iMacTia]

So far at Shopify, we didn't see much value in the privacy checks. Most of our packages have this option disabled and when we do have them enabled, people mostly fix this kind of violation mostly by moving files around, without improving the APIs, or, even worse, people just record the deprecations and forget about them.

This sounds to me like a cultural issue.
There are 2 main reasons why we decided to start using Packwerk in my org:

1. Have clear ownership over the code (i.e. each package is owned by one team)
2. Have clear boundaries between packages.

The former is a great reason to embrace Packwerk and it also helps reduce the cognitive load for new starters, but if that's all you're trying to achieve, then I agree that enforce_privacy isn't going to be interesting/useful for you.

In our case though, we also wanted to take this opportunity to apply Hexagonal Architecture patterns inside our application and decouple the different packages to reduce the entanglement in our application.
I always tell my team that, in an ideal world, if we decided to take one of our packages/components and make it a (micro)service, that should be a really easy thing to do.

But if the app/public folder is full of models, controllers and other framework-specific stuff, then I really don't see how this could possibly be achieved.
Resolving each privacy violation without simply moving files around is exactly how the codebase can evolve and improve over time, and the team can learn and experiment with different patterns to properly decouple the packages.

I love this image from the Packwerk USAGE doc, because I think it illustrates very clearly the ultimate objective of using Packwerk to achieve a healthier codebase:

[rafaelfranca]

Thank you for the opinion, of course that is a cultural issue.

But this proposal isn't about trying to pin point what kind of issue it is. Is about what to do with a feature that Shopify isn't seeing any value and it is actually getting in our way to create better, maintainable code.

I'm well aware why we introduced that feature, but it isn't cutting for us. Keeping things are is isn't not going to be an option, so maybe you could provide some suggestion of which option we should go with? Or maybe another option?

Thanks!

[iMacTia]

I totally get the problem with investing time on something that isn't seen as a priority, but I wanted to clarify why the enforce_privacy option is so important for any team that is looking to invest time into improving the quality of the codebase.

I'm actually glad you started this discussion as I'm also not totally convinced by the single public path.
Personally, I'm a fan of proposal 1.1, as it would allow flexibility around where the public constants are defined and would potentially allow teams to structure their packages as they best see fit.

The public_path option could stay around though, both for backwards-compatibility and to provide a nice shortcut to avoid adding # @export comments if all the "public" constants are in the same place.

How does that sound?

[rafaelfranca]

I like that idea a lot!

[exterm]

Caveat: I designed the first versions of packwerk, but I am not currently using it. You may treat my opinions as historical context.

Origins

Packwerk was first intended to be purely a dependency management tool. We added privacy enforcement to drive adoption, because that was what people were asking for. In the metaphor of carrots and sticks, privacy is sugar. It's easy to understand has broad appeal, but it may not actually be good for you. There are a lot of intricacies to making things private and public within a Rails application that can be, even excluding cultural issues, difficult to resolve.

However, I've been convinced back then and still am that dependency management is by far the more useful mechanism. It is a lot less likely to lead to bad design decisions. Also, it is much easier to establish conventions for public/private (e.g. in naming) than it is for dependencies. To emphasize, I do think the value proposition of packwerk is in dependencies, and privacy as an option is a nice cherry on top.

My Preference

For the above reasons, my preferred solution is 4. It allows us to focus the core of packwerk on the core value proposition, while also establishing the plugin mechanism and having people use the privacy implementation of their choice if they feel like it's the best option for them.

Explicitly Exporting Constants

I think explicitly exporting constants would only exacerbate the problem lined out by Rafael. In my opinion, having all kinds of things in the public folder is a smell. If we intentionally design and implement public interfaces, things in the public folder will be dedicated to that purpose. So there shouldn't be a broad array of types of things in there. Unless we're talking about cross-package inheritance - but I think that warrants a separate discussion.

If we change the mechanism from a public folder to "export" annotations, we will make the public interface a lot more implicit. IMO it will actually make it easier to say "I want to use this thing but I can't, so I'll just export it". Someone trying to judge the quality of the interface will not easily see that there are different kinds of things in the interface, because there isn't one place to look at. There also isn't one place to look at for people that want to use a package.

Cultural Problems

It was brought up that the problem discussed here is a cultural one.

IMO there are no purely cultural problems in software development, just as there are no purely technical ones above a certain scale.

Yes, there is a cultural component here. But tools shape culture (and vice versa), and packwerk was very much designed for that purpose. Part of this discussion should be how we want the privacy mechanism to shape people's behavior.

[exterm]

I want to add - you could say „people putting all kinds of things into the public folder“ is a cultural problem, but you could say as well that „a naming convention is not good enough, we need tooling to enforce privacy“ is a cultural problem as well

[alexevanczuk]

This has been a really interesting and thoughtful discussion, thank you everyone for participating and sharing your thoughts!

I have a proposal that I think might be satisfying based on what I'm reading in this thread. What if, as a first step, we support a plugin architecture to packwerk checkers and move privacy protection, with its behavior unchanged, into a gem. This would require no behavioral change for consumers – they simply bump packwerk to 3.0 and add the gem to their gem file, at which point packwerk should continue to operate as normal. This does a couple of things:

1. It can support a variety of other kinds of packwerk/modularization checks between packages, which is something Gusto has been invested in for a while (https://github.com/rubyatscale/package_protections) and could allow folks to extend packwerk in new and interesting ways.
2. It can allow shopify, or other organizations that don't find privacy enforcement valuable to not need to maintain its complexity. Clients have flexibility to choose what they want.
3. We can continue to have the discussion about how the privacy checker could work AND if there are strongly differing opinions on how it should work, packwerk could support different implementations of it without needing to fork all of packwerk. (In fact, privacy checking already supports multiple implementations of noting something as private or public via the public path and via enforce_privacy being set to a list of constants. No reason we couldn't support several options with a reasonable default if that is what best serves the community.)

How do people feel about having packwerk work with a plugin system and pulling privacy enforcement into a gem and iterating on its behavior independently?

[alexevanczuk]

Separately, I wanted to share a bit more what we discussed as a team at Gusto. Just to clarify -- I think that this is independent of the decision to move it into a plugin which is why this is a separate thread.

At Gusto we are really invested in the privacy checking component of packwerk, and want to continue to improve it. We feel like there are multiple separate, independent problems we are trying to solve and requirements we are trying to enforce:

1. It should be obvious and clear to consumers what public API is available.
2. The mechanism to mark something as public should be easy to use while also discouraging spurious, thoughtless promotion to public API.
3. Packwerk should not be hard to use in a vanilla Rails app, and it should at least have a configuration option/path that works with a vanilla Rails app.
4. We would like to be opinionated about how public API is expressed while...
5. ... while offering different clients with different needs a rich way to express their public API

I like this framing because it decouples the problems from the solutions (annotations, public folder, etc.). Are there other problems/requirements that I'm missing here?

If not, what we generally landed on as a team was the use of a magic comment at the top of a file to denote a file as public. Here's how it does or can solve the problems above:

1. I was imagining a CLI like bin/not_sure_where_the_cli_would_live public_api path/to/package which would open up ruby-doc like website that lists out the fils that are public.
2. As discussed, this is a cultural AND tooling issue. Appropriately, at Gusto we've approached this from a sociotechnical point of view. This means (A) we have (or want) tooling to prevent certain types of public API, such as no ActiveRecord at the public boundary and (B) invest heavily in social and cultural work to educate and evangelize what quality APIs look like. More importantly, use_packwerk already makes moving something to public trivial, so I don't think this tactic changes that.
3. This approach is invisible to Rails and all Rails conventions can be followed as normal.
4. Well this is the opinion after all
5. I'm not 100% convinced yet, but I am thinking we might want to keep the public folder around as an option, but perhaps migrate it to option (2). My preference would be to see if we can solve everything that people want from a public folder using other tactics. I'm very open to the idea of keeping around the behavior as is and I can see it as still being valuable for a lot of organizations.

[mclark]

A readable and navigable site is very cool! But if we do that, I'd also like to keep a simpler representation that can easily be searched in the IDE.

I'm just spitballing here but maybe each package could have something like a public.rbi file that includes all the modules, classes, signatures (if available), and documentation? Then the fancy navigable site could be used for truly publicly accessible documentation.

[bscofield]

💯 to the simpler representation; doc sites are lovely, but in our experience engineers don't fire them up when they're in the middle of writing code. Something close to plain text, in the repo, is the most effective documentation tool. (We've got a very naive version of this now -- it lists the files in the public interface in the pack's README, which works because our files are descriptively-named)

[alexevanczuk]

Yeah I hear you both -- some sort of simple, plain text representation of public API can be really helpful to the actual way folks will use this tool. This is a bit more akin to option (3) in Rafael's original post (although it has its own downsides). Here's a possible idea: what if the thing that got the list of files that are public API to be fed into the "site generator" ALSO has a mechanism to output that as a plain text list, so documentation is "auto-generated." Then folks can enable a CI check that verifies that documentation is up to date, if they so choose. That being said... let's keep pondering this and collaborate when it comes time to actually change the behavior.

[iMacTia]

@alexevanczuk sounds like a plan! I recently went through something similar with Faraday where we exported all adapters into separate gems and kept them as "default plugins" in v1.x

The only advice I have is to avoid keeping the "default plugins" in Packwerk's Gemfile if there's any plan to make changes to Packwerk, as this can cause a lot of headaches around version dependencies. Bundler won't in fact allow the dependency/privacy gems to specify a version constraint against Packwerk, as they'll already be in Packwerk's gemspec.

It's a tricky situation where the dependency constraint on the bundle is actually reversed, we need to be careful about that

[alexevanczuk]

Ah yes, of course. We'll probably have to skip step (3) and have it being included by default (but able to be turned off) implemented not as a gem but as a sub library within packwerk, then we can pull it out when the time comes.

[alexevanczuk]

@rafaelfranca

I've created three PRs to move towards extracting privacy out as a plugin:
#232
#233
#234

Once these land, I think I'll have enough to extract it out as a gem plugin, then I can put up a PR in packwerk that exposes some simple interface(s) for registering the plugin. At that point I can delete the implementation from packwerk and reference the gem.

At that point if we want, we could choose to remove it from packwerk entirely and publish a major version bump letting folks know they need to include the extracted plugin in their Gemfile to preserve the existing behavior.

[alexevanczuk]

Hey folks – wanted to share that this work is wrapping up. You can see the PR here that finishes up this extraction: #247

The packwerk privacy extension is being extracted here rubyatscale/packwerk-extensions#1

Let's move discussion about improvements to the privacy checker to here: rubyatscale/packwerk-extensions#2