-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug Report] Dependency violation not flagged when Zeitwerk collapsing is used #406
Comments
Packwerk uses its own constants resolver. The last version of Zeitwerk provides Zeitwerk::Loader#all_expected_cpaths that could help Packwerk honor user configuration like that by delegating that resolution to the Rails autoloaders. |
Is this something that would be a straightforward swap out? We're in the fairly early days of our project so want to enforce the boundaries from the start. If this is a fairly easy fix I could try and take a look but I currently know nothing of the internals of Packwerk |
This is acknowledged in the README:
I also hit this problem while trying to get a similar namespace to the one you mentioned 😭 Although I ended up giving up, while investigating I came across this gem which might be of interest to you. |
@Catsuko yeah I found that snippet yesterday afternoon :( I'll take a look at that other gem though thanks |
Indeed the issue is:
Constant resolver is what we use to map constants to files, and unfortunately I don't think we want to support this use-case. |
No, no, it has always been public API. The rationale is that it would be a lost race to abstract all the features into new configuration points in Rails, so we preserve See, for example the section for custom namespaces, collapsing directories for STIs, eager loading individual directories, etc. Packwerk boots the application, and for the same price, it would be more complete and aligned to resolve constants using the autoloaders. I wrote |
It seems to me that There are other major things that could be done to improve packwerk - switching to native Prism parsing for example - which are not currently being tackled. Packwerk is mainly using |
I am interested since I really want to use collapse however I don't quite understand how you would use My understanding from this thread is that when finding paths for constants, rather than using |
The natural map is from file system path to constant path. Because the file system is the input, from there, you compute the expected constant path at the spot. You cannot go from constant path to file system. Now, when scanning source code, Packwerk needs to know where is the constant supposed to be located. So, from that initial map, you can go in the other direction too. However, when you do, take into account namespaces may be spread in multiple directories and, in the case of explicit ones, a Ruby file. So, inverting the map would need to account for collections. There is also the edge case of shadowed files. They can also produce collections for files that are not necessarily namespaces. |
Ah I guess that is why the constant resolver will raise an error on duplicate files. Since Packwerk will check this as part of the validation step, it should simplify the problem if we keep the same behaviour. |
I read that code a while back and could be mistaken, but I think that only deals with files with ".rb" extension. That is different (addresses shadowed files, I believe). But, if Packwerk does not deal with namespace ownership, then directory entries can be ignored. |
@fxn If you had any time to take a look, I'd be eager to hear if this lines up with what you were thinking 🤔 |
Hey @Catsuko! In principle, the retrieved information and usage seem correct to me. People from the Packwerk project would be better informed to have an opinion on the overall patch. |
Description
A package calling another package is not triggering a violation
To Reproduce
app/modules
folderpackage.yml
files enforcing dependenciesbin/packwerk check
Expected Behaviour
A violation error to be raised
Screenshots
I've created a reproduction of the issue here https://github.com/pareeohnos/packwerk-issue
Version Information
Additional Context
The project structure contains modules in the
app/modules
directory. Within each package, each sub-directory is collapsed in Zeitwerk, so for example the directoryapp/modules/package_one/services/service_one.rb
is loaded itsPackageOne::ServiceOne
instead ofPackageOne::Services::ServiceOne
.I've tried removing the dependency on
.
and it triggers violations, but this is because theApplicationController
andApplicationRecord
classes are both located in the root of the project - it still makes not mention of the cross package violation.If I remove the collapsing altogether, it starts to work correctly but this causes issues in our application so we need the collapsing.
The text was updated successfully, but these errors were encountered: