-
Notifications
You must be signed in to change notification settings - Fork 994
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
Fixes #37102 - webpack 5 #9834
Fixes #37102 - webpack 5 #9834
Conversation
package.json
Outdated
"node-sass": "^4.5.0", | ||
"optimize-css-assets-webpack-plugin": "^3.2.0", | ||
"mini-css-extract-plugin": "2.7.5", | ||
"node-sass": "6.0.0", |
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.
Is this an update we can do independent of the webpack change?
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.
no - sass-loader
only supports node-sass
v6 from sass-loader
v11.1.0
and sass-loader
v11.0.0 only supports wepack 5
package.json
Outdated
"cross-env": "^5.2.0", | ||
"css-loader": "^0.23.1", | ||
"css-loader": "4.3.0", |
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.
Is this an update we can do independent of the webpack change?
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.
css-loader
4 requires webpack 4, so we can't do it separetly
I've tried to pick out a few spots where my goal is to identify any changes we can make independently so that we are able to do the small update round trip with packaging and then reduce the scope of changes required at one time for this PR. |
01eca52
to
f19a2a4
Compare
app/helpers/reactjs_helper.rb
Outdated
global_css_tags(global_plugins_list).join.html_safe | ||
def js_tags_for(requested_plugins) | ||
requested_plugins.map do |plugin| | ||
javascript_tag("window.tfm.tools.loadPluginModule('/webpack/#{plugin.to_s.tr('-', '_')}','#{plugin.to_s.tr('-', '_')}','./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.
In global_js_tags
you use plugin[:id]
and here just plugin
. That should be consistent.
app/helpers/reactjs_helper.rb
Outdated
def webpacked_plugins_with_global_js | ||
global_js_tags(global_plugins_list).join.html_safe | ||
def other_webpack_plugin(plugin_name, file) | ||
javascript_tag("window.tfm.tools.loadPluginModule('/webpack/#{plugin_name.to_s.tr('-', '_')}','#{plugin_name.to_s.tr('-', '_')}','./#{file}_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.
Given the repetition of this conversion I think we should have a common way for it. I'm not exactly sure where that should live, but the Foreman plugin code feels like a natural place. Perhaps we already have such a thing.
app/helpers/reactjs_helper.rb
Outdated
end | ||
def get_webpack_foreman_vendor_js | ||
if ENV['RAILS_ENV'] == 'production' | ||
javascript_include_tag("/webpack/#{File.basename(Dir.glob('public/webpack/foreman-vendor*production*js')[0])}") |
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.
Please avoid globbing for these methods. If there's some left over file it may end up using the wrong file. It also creates additional IO for every request at runtime. Previously we had some JSON file which had the mapping. Isn't that something that can still be done?
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.
Previously we had to create the JSON manifest files to make rails helpers to work correctly (it's a hack built upon the webpack-rails
gem). Now that we don't use the gem and don't reference the files from Ruby code, I don't see a good reason to keep that special configuration in webpack to generate the manifest file.
This concept inspired heavily b jsbundling-rails gem, which is the default option for rails7. The idea behind it is to reduce knowledge of the JS building process and serve the files as pure static files.
In our specific case, there always should be only one vendor file in the folder, but we don't know the hash ID for it. Would it be better to assert single file here, and prevent the app from loading in case more than one file exists?
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.
Globbing can be unpredictable. sprockets also has a random component in its manifest filename and they at least have a warning if multiple files are found:
And you still haven't addressed my concern that this isn't stored in the application program, but loaded every time. I don't insist on a manifest, but your current approach causes additional IO and can make upgrades less predictable (because there can be a point where the new files are deployed, but the application not yet restarted).
I'm not sure what the best place is, but an application initializer would be my first thought.
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.
And you still haven't addressed my concern that this isn't stored in the application program, but loaded every time. I don't insist on a manifest, but your current approach causes additional IO and can make upgrades less predictable (because there can be a point where the new files are deployed, but the application not yet restarted).
What about having the filename cached once?
Something along the lines of:
def self.manifest_file
logger.warn("too many foreman-vendor files") if Dir.glob('public/webpack/foreman-vendor*production*js').length > 1
@manifest_file ||= "/webpack/#{File.basename(Dir.glob('public/webpack/foreman-vendor*production*js')[0])}"
end
def get_webpack_foreman_vendor_js
javascript_include_tag(self.manifest_file)
end
This would make the upgrade predictable (until the next restart) and avoid extra IO.
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.
I would be worried about multiple workers. When Puma forks of a new worker, would it determine the manifest file again? In production mode I would expect it to be part of an initializer so it happens before forking.
And a small nit that I would use:
def self.manifest_file
@manifest_file ||= begin
files = Dir.glob('public/webpack/foreman-vendor*production*js')
logger.warn("too many foreman-vendor files") if files.length > 1
"/webpack/#{File.basename(files.first)}"
end
end
Because your logger statement would still glob on every request.
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.
I was thinking of doing:
def read_webpack_manifest
root = File.expand_path(File.dirname(__FILE__) + "/../..")
file = File.read(root + '/public/webpack/manifest.json')
JSON.parse(file)
end
def get_webpack_foreman_vendor_js
data = read_webpack_manifest
foreman_vendor_js = data['assetsByChunkName']['foreman-vendor'].find { |value| value.end_with?('.js') }
javascript_include_tag("/webpack/#{foreman_vendor_js}")
end
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.
@MariaAga The whole idea is to get rid of the manifest, but if we decide to leave the manifest we shall use it of course.
app/helpers/reactjs_helper.rb
Outdated
if ENV['RAILS_ENV'] == 'production' | ||
javascript_include_tag("/webpack/#{File.basename(Dir.glob('public/webpack/foreman-vendor*production*js')[0])}") | ||
else | ||
javascript_include_tag("/webpack/#{File.basename(Dir.glob('public/webpack/foreman-vendor*development*js')[0])}") |
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 implies in testing we also use the development version. Is that correct?
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.
The filename depends on NODE_ENV
should I change RAILS_ENV
to NODE_ENV
?
can NODE_ENV
be something that is not production
or development
?
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.
Not sure about NODE_ENV
. Like with #9834 (comment), can't we write some JSON file at a predictable location with the filename? I suspect that determining this at runtime is likely to cause problems at some point.
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.
As far as tests go we don't need webpack file to be loaded anyway (we don't do integration tests using RAILS_ENV=test
).
As I have mentioned before: since we need a single file there anyway, would it be OK to assert and prevent loading the app in case more than one file exists? Having the manifest file requires knowledge transfer between the webpack and rails parts of the code, and I try to simplify both webpack configuration and rails loading process.
In a long term, I think this part also should be part of the foreman federated module Maria already mentioned in the thread. This should remove the need to know the exact filename entirely.
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.
Shouldn't it be:
javascript_include_tag("/webpack/#{File.basename(Dir.glob('public/webpack/foreman-vendor*development*js')[0])}") | |
javascript_include_tag("/webpack/#{File.basename(Dir.glob('public/webpack/foreman-vendor*#{ENV.fetch('NODE_ENV', 'production')}*js')[0])}") |
Should I be able to npm install at this point? I am seeing the following:
|
@ehelms Seems to work for me. Is your katello patched as well? Did you wipe node_modules before npm install? What version of node do you have? |
No Katello involved in my test. I tried this with 18 and 16 as that is what I had local. Perhaps I made a wrong assumption that this would already enable doing it with newer Nodes. I can re-test with 12. |
I have node 14, but I can't tell if that has any impact |
Node 14 is recommended and tested. Making node 16+ available will be possible after this PR but requires some changes that should be done it a new PR. |
Looks like this also currently requires Node 14?
If yes, and there is no way to support Node 12, we will have to consider that transition in our build environment. It would be nice to be able to only do one change at a time. We need to understand if that is possible or impossible. |
@MariaAga , can we use @ehelms , can you try locally if you can work with lower version of the plugin? |
Looks like it's a no go, but due to webpack itself:
|
Based on the nodejs 14 requirement, I was curious to, from a build perspective, look at if our packages would build against 14 and then if Foreman would. I haven't tested the results, but everything built (and thanks to Copr) I have a repo with only those nodejs and Foreman package in it: https://copr.fedorainfracloud.org/coprs/ehelms/foreman-nightly-staging/ |
I took the Foreman build here for a test drive and poked around at the UI -- I found no issues. This leads me to believe we can do an update to Node 14 in our build environment with our current code base prior to this change and reduce the friction. Looking back at history, [1] was the reason we had ended up in a chicken-egg problem with prior attempts because Node 14 in the RPM environment came with an older NPM. However, I believe that we can update to Node 14 as is cleanly and that [1] is not as relevant to get to Webpack 5 (and can be a nice to have later on). @MariaAga @ShimShtein can you confirm? |
theforeman/tool_belt#569 -- this will swap build environment to Node 14. |
Can you rebase this? I've done a round of updates to Node 14. |
f19a2a4
to
ce1dda6
Compare
Thanks everyone for your input, Updated some loading order logic so Jquery should work as expected now. The PR still needs some cleaning up + info comments + test fixes |
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.
Nice work @MariaAga !! thanks for working hard on getting this in.
Something that we should pay attention to regarding dev environment compilation,
I made a change in a string and after 8 seconds the webpack compiled,
later on regular refresh the string didn't change, only after hard refresh it did.
after that I changed the string again and even when webpack got compiled successfully the page with the string remained the same.
is there any caching mechanism we are using?
Thanks
import componentRegistry from '../components/componentRegistry'; | ||
|
||
// Mounts a component after all plugins have been imported to make sure that all plugins are available to the component | ||
export const AwaitedMount = ({ component, data, flattenData }) => { |
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.
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.
Was this compiled with foreman-js 12.2.2? If so, that's the real culprit, not this line.
12.2.3 should fix everything.
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.
should we drop the version pin to v12.2.0 now? https://github.com/theforeman/foreman/pull/9834/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R24 ?
going to try with v12.2.3
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.
12.2.3 should be fine. only .1 and .2 are broken.
I'll drop the pin, good catch
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.
yeah looks like this is fixed in the latest commit here and using v12.2.3
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.
apologies, there was an issue that plugins weren't compiling well using bundle exec rake webpack:compile
it worked with npx webpack --config config/webpack.config.js
and now the issue is visible again
app/views/layouts/base.html.erb
Outdated
@@ -33,18 +30,20 @@ | |||
</script> | |||
<%= javascript_include_tag "locale/#{FastGettext.locale}/app" %> | |||
<%= locale_js_tags %> | |||
|
|||
<%= yield(:head) %> |
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.
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.
@MariaAga Can you check if clicking on the kebab toggle in the host details page fails for you as well?
I got some error regarding onKebabToggle
being undefined
, I wonder if there is some issue with the React Context provider.
verified and it works on the develop branch.
I played used this while working on theforeman/foreman_monitoring#94 and as a developer there it appeared to work as I expected. |
cba583b
to
a7c624a
Compare
Update: Works for me on the latest commit with @theforeman/vendor v12.2.3 |
|
a7c624a
to
0e88bf5
Compare
|
def javascript_include_tag(*params, **kwargs) | ||
# Workaround for overriding javascript load with webpack_asset_paths, should be removed when webpack_asset_paths is removed | ||
if kwargs[:source] == "webpack_asset_paths" | ||
kwargs[:webpacked] |
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.
There's no super
call here, so it does nothing. Is that intentional, or did you mean to unconditionally call super(...)
?
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.
we dont want to call super cause it returns a script tag,
and the webpacked key contains for example: webpacked=>"<script>\n//<![CDATA[\nwindow.tfm.tools.loadPluginModule('/webpack/foreman_remote_execution','foreman_remote_execution','./index');\n//]]>\n</script>"
which we declare in: webpacked: webpacked_plugins_js_for(plugin_name.to_sym)
2698590
to
e8f9435
Compare
e8f9435
to
e06460f
Compare
@MariaAga and all reviewers: I'm proposing we set a date to merge this and deal with any fallout after. We should announce this date on Discourse and other places so people are aware of it. I'm proposing Monday 29 or Tuesday 30. That gives us a full week to iterate on it and prepare, while also giving us a week before FOSDEM + cfgmgmtcamp. Looking at the 3.10 schedule we'd have about 2 weeks until stabilization week to deal with any fallout, or roll back if it's really too bad. How does that sound? |
.packit.yaml
Outdated
@@ -18,7 +18,7 @@ downstream_package_name: foreman | |||
|
|||
actions: | |||
post-upstream-clone: | |||
- "wget https://raw.githubusercontent.com/theforeman/foreman-packaging/rpm/develop/packages/foreman/foreman/foreman.spec -O foreman.spec" |
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.
we should not forget to drop the [HACK] use evgeni's temporary repo with webpack5 builds
commit before merging (we can also do so now, the only downside would be failing packit builds, but that's IMHO fine)
Can we aim for Friday the 26th instead? That would allow us to come in fresh on Monday and see what the pipelines and everyone has to say about the result. |
I'm not a fan of merging something potentially breaking on Friday, but I can see arguments for letting pipelines run. I can't speak for everyone, but my Monday is also pretty full with meetings which why I also said Tuesday. Friday would be OK with me. If people get stuck with things on the weekend, they can submit PRs and we can merge them while waiting on pipelines during meetings. |
e06460f
to
ecf7586
Compare
9333d6c
to
fa778e0
Compare
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.
Let's break some pipelines and development environments by merging this :)
As a reminder, https://community.theforeman.org/t/webpack-5-merge-this-friday-2024-01-26/36581 recommends removing your node_modules
directory entirely and run npm install
fresh.
Details and discussion here: https://community.theforeman.org/t/resurrection-of-the-client-side-infrastructure-upgrade-effort/32340