Don't report violations until told to do so

[exterm]

What are you trying to accomplish?

Packwerk is not opinionated about where you draw boundaries and which direction your dependencies have.

It's a tool that helps you enforce or move towards an architecture that you come up with.

So it shouldn't return errors unless a constraint specified by the user is violated.

Right now we set enforce_dependencies: true in the root package in packwerk init, with no list of dependencies (defaults to empty). This means that any newly introduced package may cause packwerk to report violations, even before the user has specified how this new package fits in with the application's architecture.

For example, see my blog post about enforcing lib/ to not have dependencies.

What approach did you choose and why?

Do not enforce anything on the root package by default. This is the simplest solution. Packwerk doesn't enforce anything by default. That means only user-specified constraints are enforced.

What should reviewers focus on?

Is there any documentation I need to change along with this?

Type of Change

• Bugfix
  • I would call this a bugfix to packwerk init, but at the very least it's a breaking change to it.
• New feature
• Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Additional Release Notes

• Breaking change (fix or feature that would cause existing functionality to change)
  • Not a breaking change for existing installs.

Checklist

• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• It is safe to rollback this change.

[gmcgibbon]

Personally I think that Packwerk should be enforcing dependencies by default. Without doing this, Packwerk holds no value in the codebase. If the developer chooses to not enforce dependencies at this time, it is their decision, and it should be done explicitly IMO. It may be annoying to have violations in new packages as a result of this behaviour, but I think it is well worth it to have a stricter default.

What we may want to do here though is uncomment the dependencies key and make it an empty array. In theory, the application package should not have any dependencies, but that is up to the developer to decide.

cc @Shopify/packwerk if anyone else has opinions.

[exterm]

Hi Gannon!

I appreciate your thoughts on this, but respectfully disagree. Let me see if I can convince you.

What we may want to do here though is uncomment the dependencies key and make it an empty array.

That would be a slight improvement IMO but doesn't really address my concern.

should be enforcing dependencies by default. Without doing this, Packwerk holds no value in the codebase.

🤷 Packwerk doesn't know anything about the structure of your codebase, so it can't enforce anything out of the box. You have to define packages. Who says you will want to enforce dependencies on the root package? You may want to enforce dependencies only on one or multiple of the smaller packages you create.

The design of this tool is at its core focused on avoiding false positives, and in many realistic workflows these violations on the root package that spring up when you start packaging parts of your application do share a lot of properties with the false positives we tried to avoid in other places. That's why I chose this specific title and description for the PR. Violations of rules specified by the user are useful output. Everything else is potentially confusing noise.

Rubocop comes with cops that are enabled by default, which makes sense because there are some statements that we can make about Ruby code that are universally true. Packwerk is concerned with the shape and architecture of applications though where we can make a lot less assumptions. If we do want to enforce things out of the box the one obvious property for all Rails apps seems to be the one I describe in my blog post - libraries shouldn't depend on the application. But I don't think "the root package doesn't depend on anything" is a valid assumption to make for all Rails apps.

Maybe the cleaner solution is not to define a root package out of the box in the first place? I'm not sure. I find the root package.yml to be a nice template, and having it also makes it so all code is in some package by default.

IIRC the reason we introduced the root package.yml to the generator was so that we could have all code in a package without having to add an implicit root package (which was the original solution).

[gmcgibbon]

👋

Who says you will want to enforce dependencies on the root package?

We chose to enforce dependencies on the root application package in the monolith because references to packaged code will leak into the app otherwise, and that creates unnecessary / undeclared coupling. In terms of the overall dependency graph of an application, that's not great IMO.

Violations of rules specified by the user are useful output. Everything else is potentially confusing noise.

That's a valid point.

libraries shouldn't depend on the application

I read some of your blog post before writing my first comment -- thanks for linking it. I think we have a fundamental disagreement here as well. In terms of Rails engines / apps, Rake tasks are part of the library and will often reference application code. These tasks belong to packages though, the root application should have little to no code. Regarding all other lib code within an engine, I agree it shouldn't reference app code. Perhaps specific subdirectories of lib could be packages, as long as the intention is to extract?

Maybe the cleaner solution is not to define a root package out of the box in the first place?

Perhaps, but then Packwerk does nothing valuable by default after init. Maybe that's ok given your earlier comment on everything not configured by the developer is potentially confusing noise. Tools inspired by Packwerk (eg. packs) have generator-like CLI subcommands to create packages, which we could introduce to make package creation more intentional. Overall, I think my objection is more the default of no enforcement, and how opinionated Packwerk should be by default.

I'll think a little more about this and get back to you.

[exterm]

Tools inspired by Packwerk (eg. packs) have generator-like CLI subcommands to create packages, which we could introduce to make package creation more intentional.

👍🏼 👍🏼

Rake tasks are part of the library and will often reference application code.

Rake tasks are, IMO, not library code, and the fact that they live in the lib folder seems like a bit of an accident. The fact that Rails 7.1+ autoloads lib, but omits a few directories specifically, confirms that lib is an overloaded concept. I didn't want to go into quite that much detail in my blog post to keep it approachable; now I'm wondering if I should add a note on that...

If rake tasks reference the application, they are explicit entry points into application code. I agree they should not be treated as library code by packwerk.

[exterm]

Two ideas:

1. Make packwerk unopinionated (which it mostly already is), put opinions into a separate gem. That gem could then also contain generators to enable people to replicate Shopify's usage in Componentization (as asked for in Documenting the happy path for implementation via Rails engines #361)

2. Make packwerk opinionated by default but let users opt out - e.g. packwerk init --bare or something like that. Arguably that isn't too different from packwerk init followed by editing the configuration, but it's a little nicer.

[gmcgibbon]

After chatting about it with the team, I don't think we have an overly strong opinion. We want the package setup to make sense by default, and turning on dependency checks by default in the root package infers this is correct, and subsequent violations are correct. This has the downside of causing confusion as soon as the developer adds another package, raising a lot of unhelpful errors. This isn't setting developers up for success, and this initial difficulty is the root problem this change is trying to solve.

While I have concerns about not enforcing anything by default, I think you've convinced me it is more correct behaviour. More advanced users of Packwerk can turn it on by default if they know what they're doing, and they want this behaviour. I also think that if we add a package generator in the future to the gem, we should enforce dependencies by default for those. That's not really a concern for this change though.

[tylerwillingham]

I also think that if we add a package generator in the future to the gem, we should enforce dependencies by default for those. That's not really a concern for this change though.

Just voicing support for this. My team has created some bespoke (but lean) generators that effectively generate a package.yml that has enforce_dependencies: true and dependencies: [] plus some metadata that we care about and an opinionated folder structure - so we've seen the use in some default opinions

[exterm]

I also think that if we add a package generator in the future to the gem, we should enforce dependencies by default for those.

Yep, full agreement from me too! Thanks for review, deliberation and acceptance, Gannon 😄