Trying out `packwerk` in non-zeitwerk app - Feedback needed!

This message was imported from the Ruby/Rails Modularity Slack server. Find more info in the import thread.

<@U70NN25TNJA> You can now try out packwerk in your non-zeitwerk app.

It might be a rough experience at first, but let me know your feedback!

https://github.com/alexevanczuk/packs/blob/main/EXPERIMENTAL_PARSER_USAGE.md

Message originally sent by slack user U70NN25TNJA

Awesome, thanks!! :slightly_smiling_face: We’ll try and get you some feedback within the next week.

Message originally sent by slack user U70NN25TNJA

Well, it turns out my colleague <@U72GGKIGPTW> and I couldn’t resist taking it for a spin today :slightly_smiling_face:

On the whole, it seemed to Just Work™! Super excited for where this is going. Some observations:

• We had to add dependencies for newly created packs to the root package.yml. In our experience with another Rails app we’ve been using as a place for packwerk experimentation, the root package.yml didn’t have any packs in. For what it’s worth, this actually felt kinda nice - by forcing us to add the depedencies, it felt a little less magical.
• We were definitely impacted by the monkey-patching limitations. :sweat_smile: From our perspective, explicitly listing the monkey-patches in the packwerk.yml seems like an interesting compromise. This might be a result of us being opinionated about the usage of monkey-patching, but it seems helpful to force folks to explicitly declare when a pack is changing things it doesn’t own. It’d introduce a little friction, and hopefully stimulate conversations over the need of doing so. For instance, one could imagine a danger-packwerk check that adds a "Are you sure you want to do that? :upside_down_face:" comment to merge requests…
• When a packwerk.yml file does not exist, running packs delete-cache will delete the current directory :grimacing: We’re assuming that this has something to do with it not being able to find a tmp directory and just falling back to the current working directory (or something like that!)
• When we didn’t use the experimental parser, packs crashed:

thread 'main' panicked at 'Found two constants with the same name: Constant { fully_qualified_name: "Foo", absolute_path_of_definition: "/Users/mike.stallard/workspace/checkr/app/routes/foo.rb" } and Constant { fully_qualified_name: "Foo", absolute_path_of_definition: "/Users/mike.stallard/workspace/checkr/app/models/foo.rb" }', src/packs/parsing/ruby/packwerk/constant_resolver.rs:52:21
stack backtrace:
   0:        0x1006a4a64 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h819e9cbdf1a9e730
   1:        0x1006be3f0 - core::fmt::write::ha5e9bf3131ecb7c0
   2:        0x1006a1b40 - std::io::Write::write_fmt::h414ce9994bf17404
   3:        0x1006a4878 - std::sys_common::backtrace::print::h8072db0bbd5bcc3d
   4:        0x1006a5ea8 - std::panicking::default_hook::{{closure}}::h2c85c5b0c2ede151
   5:        0x1006a5c68 - std::panicking::default_hook::hcf2f70992d02f6fe
   6:        0x1006a6380 - std::panicking::rust_panic_with_hook::h023af7f90b47eb8b
   7:        0x1006a62b4 - std::panicking::begin_panic_handler::{{closure}}::h14283519edc1d634
   8:        0x1006a4e84 - std::sys_common::backtrace::__rust_end_short_backtrace::hc366c0b0cef5b747
   9:        0x1006a6048 - _rust_begin_unwind
  10:        0x1006dc510 - core::panicking::panic_fmt::h324f50b29db90195
  11:        0x1003e4ad0 - packs::packs::parsing::ruby::packwerk::constant_resolver::ConstantResolver::create::h601a15d7b216e9bb
  12:        0x1003e62a4 - packs::packs::parsing::ruby::zeitwerk_utils::get_zeitwerk_constant_resolver::h202216ebf2121422
  13:        0x10040ca5c - packs::packs::checker::get_all_violations::h98017f43fa83a461
  14:        0x10040c0b0 - packs::packs::checker::check_all::h47633e5377e3636c
  15:        0x100455a30 - packs::packs::cli::run::hc1fbfe4e60401be1
  16:        0x1003dbbe8 - std::sys_common::backtrace::__rust_begin_short_backtrace::h30f09d9d21283f1c
  17:        0x1003dba7c - std::rt::lang_start::{{closure}}::hc11c66ee21b158b0
  18:        0x10069c5c0 - std::rt::lang_start_internal::h8ee16b8f6c950a26
  19:        0x1003dba60 - _main
  

For what it’s worth, Foo is defined in the file in the models directory, and referenced in the file in routes with ::Foo.

• Naturally, it would be lovely to have the other nice features of the original packs tooling, such as creating packs, moving files, add dependencies, etc.
We’re curious what features you plan on adding next. We’re also happy to experiment further, and provide any more specific feedback you’re looking for.

Thanks for the feedback <@U70NN25TNJA>!!
• “We had to add dependencies…”
◦ This is a bit surprising to me – do you have enforce_dependencies: true set in your root package.yml? The default value for it should be false if unset.
• Monkey-patching limitations
◦ Sounds like we’re on the same page! It’d definitely be interesting to see what are in those lists. I’ll follow up later to figure out what sort of APIs/UX make sense.
delete-cache issue:
◦ Thanks for reporting that one :cold_sweat: . Your assumption is correct. I had a default but (due to reasoning explained in the pr) it wasn’t respected the way I thought. I’ve released a fix and added a test here https://github.com/alexevanczuk/packs/pull/62 (you can update with cargo install pks) so this does not happen again. I apologize for any inconvenience that caused :pray:
• Panic without experimental parser:
◦ Currently the packwerk parser is pretty strict about assuming each file defines one constant, and each constant is defined by one file.
◦ We could probably refine this (e.g. to ignore routes), but you may be better off just using the experimental parser anyways (especially since you don’t need to be compatible with packwerk). If there are other reasons you’re interested in trying to get the packwerk parser to work, I’d love to better understand!
• Other tooling
◦ Definitely – it’d be a much better experience for users and easier to maintain. I’m hoping to meet some of the base needs and then iterating from there.
◦ That being said – some of the tooling in use_packs might still work, but some features (e.g. moving files) generally assume you’re following zeitwerk conventions
◦ Also – I’m thinking long-term a more generalized ruby language LSP, such as the one Shopify released, can solve some of those problems, and perhaps packs can integrate with it for packs specific features.

Message originally sent by slack user U70NN25TNJA

Hey @AlexEvanczuk, happy Friday! :wave:

• re: enforce_depdenencies: true
◦ our apologies - this was our mistake from copy+pasting a package.yml to get things up and running in our non-Rails app, as there isn’t an init command yet. Everything is consistent - sorry for the noise! :grimacing:
• re: panic without experimental parser
◦ makes sense, our comment was more just around the dev experience (which is not surprise given how early days it still is!)
• re: other tooling
◦ fyi, we took use_packs for a spin in this codebase - some things work fine (e.g. create and move seemed to be fine?), however some things get a little noisy due to the dependency on the Packwerk CLI. For instance, add_dependency successfully updates a pack’s package.yml, but emits a big ol’ stacktrace due to it trying to call Packwerk in order to validate the updated yaml. Not sure if there’s anything to be done here, but thought we’d pass it along regardless.