Skip to content

Commit

Permalink
DEV: Do not manipulate theme module paths at build-time (#23148)
Browse files Browse the repository at this point in the history
Manipulating theme module paths means that the paths you author are not the ones used at runtime. This can lead to some very unexpected behavior and potential module name clashes. It also meant that the refactor in 16c6ab8 was unable to correctly match up theme connector js/templates.

While this could technically be a breaking change, I think it is reasonably safe because:

1. Themes are already forced to use relative paths when referencing their own modules (since they're namespaced based on the site-specific id). The only time this might be problematic is when theme tests reference modules in the theme's main `javascripts` directory

2. For things like components/services/controllers/etc. our custom Ember resolver works backwards from the end of the path, so adding `discourse/` in the middle will not affect resolution.
  • Loading branch information
davidtaylorhq committed Aug 18, 2023
1 parent 291749e commit 82b16f4
Show file tree
Hide file tree
Showing 5 changed files with 9 additions and 7 deletions.
2 changes: 1 addition & 1 deletion app/models/theme.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
class Theme < ActiveRecord::Base
include GlobalPath

BASE_COMPILER_VERSION = 72
BASE_COMPILER_VERSION = 73

attr_accessor :child_components

Expand Down
2 changes: 1 addition & 1 deletion lib/theme_javascript_compiler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ def append_raw_script(filename, script)

def append_module(script, name, include_variables: true)
original_filename = name
name = "discourse/theme-#{@theme_id}/#{name.gsub(%r{\Adiscourse/}, "")}"
name = "discourse/theme-#{@theme_id}/#{name}"

script = "#{theme_settings}#{script}" if include_variables
transpiler = DiscourseJsProcessor::Transpiler.new
Expand Down
4 changes: 3 additions & 1 deletion spec/lib/theme_javascript_compiler_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,9 @@
"discourse/templates/components/mycomponent.hbs" => "{{my-component-template}}",
},
)
expect(compiler.raw_content).to include('define("discourse/theme-1/components/mycomponent"')
expect(compiler.raw_content).to include(
'define("discourse/theme-1/discourse/components/mycomponent"',
)
expect(compiler.raw_content).to include(
'define("discourse/theme-1/discourse/templates/components/mycomponent"',
)
Expand Down
4 changes: 2 additions & 2 deletions spec/models/theme_field_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -259,10 +259,10 @@
)
expect(theme.javascript_cache.content).to include('addRawTemplate("discovery"')
expect(theme.javascript_cache.content).to include(
"define(\"discourse/theme-#{theme.id}/controllers/discovery\"",
"define(\"discourse/theme-#{theme.id}/discourse/controllers/discovery\"",
)
expect(theme.javascript_cache.content).to include(
"define(\"discourse/theme-#{theme.id}/controllers/discovery-2\"",
"define(\"discourse/theme-#{theme.id}/discourse/controllers/discovery-2\"",
)
expect(theme.javascript_cache.content).to include("const settings =")
expect(theme.javascript_cache.content).to include(
Expand Down
4 changes: 2 additions & 2 deletions spec/models/theme_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ def transpile(html)

expect(javascript_cache.content).to include("if ('define' in window) {")
expect(javascript_cache.content).to include(
"define(\"discourse/theme-#{field.theme_id}/initializers/theme-field-#{field.id}-mobile-html-script-1\"",
"define(\"discourse/theme-#{field.theme_id}/discourse/initializers/theme-field-#{field.id}-mobile-html-script-1\"",
)
expect(javascript_cache.content).to include(
"settings = require(\"discourse/lib/theme-settings-store\").getObjectForTheme(#{field.theme_id});",
Expand Down Expand Up @@ -406,7 +406,7 @@ def transpile(html)
)
expect(theme_field.javascript_cache.content).to include("if ('define' in window) {")
expect(theme_field.javascript_cache.content).to include(
"define(\"discourse/theme-#{theme_field.theme.id}/initializers/theme-field-#{theme_field.id}-common-html-script-1\",",
"define(\"discourse/theme-#{theme_field.theme.id}/discourse/initializers/theme-field-#{theme_field.id}-common-html-script-1\",",
)
expect(theme_field.javascript_cache.content).to include(
"name: \"theme-field-#{theme_field.id}-common-html-script-1\",",
Expand Down

0 comments on commit 82b16f4

Please sign in to comment.