Skip to content
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

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

MariaAga
Copy link
Member

@MariaAga MariaAga commented Sep 14, 2023

@github-actions github-actions bot added UI Legacy JS PRs making changes in the legacy Javascript stack Docs labels Sep 14, 2023
bundler.d/assets.rb Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
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",
Copy link
Member

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?

Copy link
Member Author

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",
Copy link
Member

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?

Copy link
Member Author

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

@ehelms
Copy link
Member

ehelms commented Sep 18, 2023

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.

developer_docs/getting-started.asciidoc Outdated Show resolved Hide resolved
app/assets/javascripts/late_load.js Outdated Show resolved Hide resolved
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)
Copy link
Member

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.

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)
Copy link
Member

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.

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])}")
Copy link
Member

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?

Copy link
Member

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?

Copy link
Member

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:

https://github.com/rails/sprockets/blob/5b040f3bec503ded8a98c0c911c7a77dd1f5bf1b/lib/sprockets/manifest_utils.rb#L40-L43

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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 Show resolved Hide resolved
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])}")
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be:

Suggested change
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])}")

@ehelms
Copy link
Member

ehelms commented Oct 11, 2023

Should I be able to npm install at this point? I am seeing the following:

$ npm install
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR! 
npm ERR! While resolving: [email protected]
npm ERR! Found: [email protected]
npm ERR! node_modules/webpack
npm ERR!   dev webpack@"5.75.0" from the root project
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer webpack@"^4.0.0" from [email protected]
npm ERR! node_modules/optimize-css-assets-webpack-plugin
npm ERR!   dev optimize-css-assets-webpack-plugin@"^4.0.0" from the root project
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

@adamruzicka
Copy link
Contributor

@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?

@ehelms
Copy link
Member

ehelms commented Oct 12, 2023

@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.

@adamruzicka
Copy link
Contributor

I have node 14, but I can't tell if that has any impact

@MariaAga
Copy link
Member Author

MariaAga commented Oct 12, 2023

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.
Also I recommend deleting package-lock.json and node_modules when testing this PR.

@ehelms
Copy link
Member

ehelms commented Oct 12, 2023

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?

npm ERR! code ENOTSUP
npm ERR! notsup Unsupported engine for [email protected]: wanted: {"node":">= 14.15.0"} (current: {"node":"12.20.1","npm":"6.14.10"})
npm ERR! notsup Not compatible with your version of node/npm: [email protected]
npm ERR! notsup Not compatible with your version of node/npm: [email protected]
npm ERR! notsup Required: {"node":">= 14.15.0"}
npm ERR! notsup Actual:   {"npm":"6.14.10","node":"12.20.1"}

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.

@ShimShtein
Copy link
Member

npm ERR! notsup Unsupported engine for [email protected]: wanted: {"node":">= 14.15.0"} (current: {"node":"12.20.1","npm":"6.14.10"})

@MariaAga , can we use [email protected] ? I see they dropped the support for node 12 in >=4.0.0: webpack-contrib/css-minimizer-webpack-plugin@88a5674

@ehelms , can you try locally if you can work with lower version of the plugin?

@ehelms
Copy link
Member

ehelms commented Oct 17, 2023

npm ERR! notsup Unsupported engine for [email protected]: wanted: {"node":">= 14.15.0"} (current: {"node":"12.20.1","npm":"6.14.10"})

@MariaAga , can we use [email protected] ? I see they dropped the support for node 12 in >=4.0.0: webpack-contrib/css-minimizer-webpack-plugin@88a5674

@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:

pm ERR! notsup Unsupported engine for [email protected]: wanted: {"node":">=14.15.0"} (current: {"node":"12.20.1","npm":"6.14.10"})
npm ERR! notsup Not compatible with your version of node/npm: [email protected]
npm ERR! notsup Not compatible with your version of node/npm: [email protected]
npm ERR! notsup Required: {"node":">=14.15.0"}
npm ERR! notsup Actual:   {"npm":"6.14.10","node":"12.20.1"}

@ehelms
Copy link
Member

ehelms commented Oct 18, 2023

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/

@ehelms
Copy link
Member

ehelms commented Oct 18, 2023

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?

[1] theforeman/foreman-js#404

@ehelms
Copy link
Member

ehelms commented Oct 20, 2023

theforeman/tool_belt#569 -- this will swap build environment to Node 14.

@ehelms
Copy link
Member

ehelms commented Oct 26, 2023

Can you rebase this? I've done a round of updates to Node 14.

app/helpers/layout_helper.rb Outdated Show resolved Hide resolved
@MariaAga
Copy link
Member Author

MariaAga commented Nov 2, 2023

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

Copy link
Member

@Ron-Lavi Ron-Lavi left a 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

app/assets/javascripts/application.js Show resolved Hide resolved
Comment on lines +4 to +7
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 }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a warning on every page about Component name already taken:
from the stack trace it's originated from this file.

image

image

Copy link
Member

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.

Copy link
Member

@Ron-Lavi Ron-Lavi Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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

Copy link
Member

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

Copy link
Member

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

@@ -33,18 +30,20 @@
</script>
<%= javascript_include_tag "locale/#{FastGettext.locale}/app" %>
<%= locale_js_tags %>

<%= yield(:head) %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just before the end of the tags we are getting now many <style> tags which I think comes from this yield:

image

as of now, I don't see an impact on existing pages.

Copy link
Member

@Ron-Lavi Ron-Lavi left a 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.

@MariaAga
Copy link
Member Author

MariaAga commented Jan 2, 2024

Also saw this issue now, that I will fix
image

@ekohl
Copy link
Member

ekohl commented Jan 2, 2024

I played used this while working on theforeman/foreman_monitoring#94 and as a developer there it appeared to work as I expected.

@evgeni evgeni force-pushed the webpack5-module-federations branch from cba583b to a7c624a Compare January 3, 2024 12:28
@Ron-Lavi
Copy link
Member

Ron-Lavi commented Jan 3, 2024

@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.

Update: Works for me on the latest commit with @theforeman/vendor v12.2.3

@MariaAga
Copy link
Member Author

MariaAga commented Jan 3, 2024

I got some error regarding onKebabToggle being undefined, I wonder if there is some issue with the React Context provider.
This is still happening with bootdisk plugin I think. I have a solution for it, will push when I'll resolve the rest of the issues (400 style tags, and deprecation text in the UI)

@Ron-Lavi
Copy link
Member

Ron-Lavi commented Jan 3, 2024

I got some error regarding onKebabToggle being undefined, I wonder if there is some issue with the React Context provider. This is still happening with bootdisk plugin I think. I have a solution for it, will push when I'll resolve the rest of the issues (400 style tags, and deprecation text in the UI)

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 with clicking the host details kebab is happening again, my plugins are:
image

@MariaAga
Copy link
Member Author

  • bundle exec rake webpack:compile is defaulting to NODE_ENV=production I'll remove/edit the suggestion to use it.
  • Instead of adding context files (like Templates/CardItem/CardTemplate/index.js and HostDetails/ActionsBar/index.js) to ModuleFederationPlugin shared field, I have wrapped the contexts with forceSingleton which resolved the issues in host details page.
  • Changed style-loader to use singletonStyleTag - https://webpack.js.org/loaders/style-loader/#singletonstyletag
    Added an id=plugin so we can track which plugin css rules come from as this option disables source maps.
    image

Procfile Outdated Show resolved Hide resolved
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]
Copy link
Member

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(...)?

Copy link
Member Author

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)

package.json Outdated Show resolved Hide resolved
@ekohl
Copy link
Member

ekohl commented Jan 19, 2024

@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"
Copy link
Member

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)

@evgeni
Copy link
Member

evgeni commented Jan 19, 2024

I'm proposing Monday 29 or Tuesday 30

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.

@ekohl
Copy link
Member

ekohl commented Jan 19, 2024

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.

@ekohl ekohl changed the title webpack 5 Fixes #37102 - webpack 5 Jan 26, 2024
Copy link
Member

@ekohl ekohl left a 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.

@ekohl ekohl merged commit 89c6104 into theforeman:develop Jan 26, 2024
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs Legacy JS PRs making changes in the legacy Javascript stack UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants