-
Notifications
You must be signed in to change notification settings - Fork 296
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
webpack5 #10735
webpack5 #10735
Conversation
<% if File.exist?("#{Bastion::Engine.root}/vendor/assets/javascripts/#{Bastion.localization_path(I18n.locale)}") %> | ||
<%= javascript_include_tag(Bastion.localization_path(I18n.locale)) %> | ||
<% end %> | ||
<script type="text/javascript"> | ||
<script type="module"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What defines this type? Is it webpack itself or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looked more correct to use module
than text/javascript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules
Foreman PR: theforeman/foreman#9834 |
19e0a3a
to
a68b8ff
Compare
app/views/overrides/activation_keys/_host_synced_content_select.html.erb
Outdated
Show resolved
Hide resolved
@@ -10,11 +10,13 @@ | |||
<% end %> | |||
|
|||
<% content_for(:javascripts) do %> | |||
<%= javascript_include_tag 'bastion/bastion' %> | |||
|
|||
<%= javascript_tag("window.tfm.tools.loadPluginModule('/webpack/katello','katello','./common_index');".html_safe) %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very similar to what js_tags_for(requested_plugins)
already does for katello
, except that this calls it for common_index
and the other does it for index
.
I'm trying to understand what common_index.js
even does. I don't see anything that includes it and it only requires ngreact
. If you don't have this, what happens? If it's needed, can it just be moved into katello's index.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It loads angular, and since angular is not needed in all of Katello I think we shouldn't move it to index.
Without angular pages are just empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Thanks for explaining.
a68b8ff
to
3e7a29f
Compare
See theforeman/foreman#9834 (comment) If this is ready to go, please create a Redmine. thanks! |
3e7a29f
to
5e1820e
Compare
looks like angular is failing on eslint |
5e1820e
to
c73aade
Compare
Core PR was merged, so this should be merged. |
@@ -14,12 +14,6 @@ class Engine < ::Rails::Engine | |||
|
|||
Bastion.register_plugin( | |||
:name => 'bastion_katello', | |||
:javascript => proc do | |||
[ | |||
javascript_include_tag(*webpack_asset_paths('katello:common', :extension => 'js'), "data-turbolinks-track" => true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This broke the "Sync Status" (/katello/sync_management
) page, as the console now contains the following error:
63:2 Uncaught ReferenceError: localize is not defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an ERB page, I wonder why we're even loading Katello JS here 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created #10873
Details and discussion here: https://community.theforeman.org/t/resurrection-of-the-client-side-infrastructure-upgrade-effort/32340