Conversation
2ef6242 to
fffb14b
Compare
|
Woah, that’s a lot of changes! |
|
It needed a bit to change how the parts worked - when I got into it, I realised we needed to solve for conflicts in naming where two pages or the layout have parts with the same name - those shouldn't matter and should work, but wouldn't have. |
|
Sheeeeesh! This is a significant change. I like the idea of the abstractions. I think this is a very difficult program to work on architecturally as is. It's hard for me to tell right now as so much of this I still haven't figured out, but I hope that this new organization for the page template metadata will make it easier to contribute new modules to Spina. |
|
The migration task crashes: lib/tasks/theme.rake:20 uses bare ThemeMigrator instead of Spina::ThemeMigrator, which raises NameError. Instead of aliasing |
|
Yeah I like that approach. Done that and fixed the rake. |
|
A few remaining items from my review:
|
|
Regarding the dev reload gap: the fix is to watch directories instead of a snapshot of files. def updater
@updater ||= Rails.application.config.file_watcher.new([], watched_dirs) do
reload!
end
end
def watched_dirs
{
Rails.root.join("config/initializers/themes").to_s => ["rb"],
Rails.root.join("app/templates/spina").to_s => ["rb"]
}
endThis also removes the boot-time globs. One caveat: custom |
|
Force pushed again to keep the PR as a single commit |
Bramjetten
left a comment
There was a problem hiding this comment.
Sectioned layout parts break. theme.layout_parts can be a Hash since #1307 (tabs in the Layout screen). The DSL can't express sections, and the migrator crashes on them — validate_part_references! does Array(@theme.layout_parts) which turns a Hash into [key, value] pairs (theme_migrator.rb:82):
MissingPartError: Theme "sectioned" references undefined parts: "[:general, [\"footer\"]]", "[:seo, [\"footer\"]]"
current_theme_name isn't thread-safe. It's a class-level ivar (page_template.rb:36), so concurrent Theme.register calls (common in multi-tenant setups that register themes at runtime) interleave: thread A's template files register under thread B's theme, and the ensure blocks restore in the wrong order. Result: parts leak across tenants, the other theme ends up empty. Rails >= 7.0 is required anyway, so ActiveSupport::IsolatedExecutionState plus a Mutex around registration fixes it. The registries (@registry ||= {}, clear_for_theme racing readers) have the same problem.
Zeitwerk test is vacuous. refute Object.const_defined?(:Homepage) passes regardless — the file would map to Spina::Demo::Homepage, not Homepage. Assert Rails.autoloaders.main.ignores?(...) instead.
Ignore is broader than needed. engine.rb:20 ignores all of app/templates; scoping to app/templates/spina avoids breaking host apps using that directory.
Theme initializers had grown into long lists of parts and view
templates that were hard to read and painful to maintain. This moves
those definitions into app/templates/spina/{theme}/, one file per page
template plus an optional layout.rb for site-wide parts.
Spina.define_template and Spina.define_layout_parts provide a small
DSL for declaring parts and repeaters without aliasing constants onto
Object. Built-in part types use symbols; custom parts keep their full
class name as a string. At boot, Spina compiles these files into the
same structures the admin already expects, so the rest of the engine
stays largely unchanged.
Part definitions are scoped to their template (and separately for
layout), so the same name can mean different things in different
places without conflict. The old theme.parts and theme.view_templates
configuration still works for now, with a deprecation warning and a
spina:theme:migrate_templates task to convert existing themes. The
migrator backs up the original initializer as .rb.bak so Rails won't
load it.
The install generator and dummy app are updated to use the new layout.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Hopefully that's the last of them :) |
Theme initializers had grown into long lists of parts and view templates that were hard to read and painful to maintain. This moves those definitions into app/templates/spina/{theme}/, one file per page template plus an optional layout.rb for site-wide parts. PageTemplate.define and LayoutParts.define provide a small DSL for declaring parts and repeaters. Built-in part types use symbols; custom parts keep their full class name as a string. At boot, Spina compiles these files into the same structures the admin already expects, so the rest of the engine stays largely unchanged.
Part definitions are scoped to their template (and separately for layout), so the same name can mean different things in different places without conflict. The old theme.parts and theme.view_templates configuration still works for now, with a deprecation warning and a spina:theme:migrate_templates task to convert existing themes. The migrator backs up the original initializer as .rb.bak so Rails won't load it.
The install generator and dummy app are updated to use the new layout.