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

[WIP] Remove foreman vendor #10342

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

MariaAga
Copy link
Member

@MariaAga MariaAga commented Oct 8, 2024

Depends on #10345 #10239

@github-actions github-actions bot added UI Legacy JS PRs making changes in the legacy Javascript stack Docs labels Oct 8, 2024
Comment on lines +205 to +211
export const visit = url => {
window.location.href = url;
};

export const reloadPage = () => {
window.location.reload();
};
Copy link
Member Author

Choose a reason for hiding this comment

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

Add comment to explain

Comment on lines -40 to +42
<%= javascript_include_tag("/webpack/#{get_webpack_chunk('bundle', 'js')}") %>
<%= javascript_include_tag("/webpack/#{get_webpack_chunk('reactExports', 'js')}") %>
<%= javascript_include_tag("/webpack/#{get_webpack_chunk('bundle', 'js')}") %>
Copy link
Member Author

Choose a reason for hiding this comment

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

Check if needed

@@ -5,7 +5,8 @@
"description": "Foreman isn't really a node module, these are just dependencies needed to build the webpack bundle. 'dependencies' are the asset libraries in use and 'devDependencies' are used for the build process.",
"private": true,
"engines": {
"node": ">=14.0.0 <21.0.0"
"node": ">=14.0.0 <21.0.0",
"npm": ">=8.0.0"
},
"scripts": {
Copy link
Member Author

Choose a reason for hiding this comment

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

look into adding NODE_OPTIONS="--max-old-space-size=8192"

@MariaAga
Copy link
Member Author

In this PR we had to update to npm 8 to get the "overrides" attribute in package.json - "Overrides provide a way to replace a package in your dependency tree with another version, or another package entirely. ".
Which we need to pin victory packages that are used in pf4 charts package. before I think because of some vendor magic pf4 used our install victory, and not the one from the pf4 node_modules, but since I'm simplifying things and moving it all to foreman it doesnt work anymore so we need to completely override victory versions, and not just installing an old one.

@theforeman/packaging are we ok to update to NPM 8+?

@MariaAga MariaAga force-pushed the remove-foreman-vendor branch from d48bcac to 005f580 Compare October 30, 2024 16:54
@MariaAga
Copy link
Member Author

@theforeman/packaging are we ok to update to NPM 8+?

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.

Not a real review, but I'd prefer to wrap up https://community.theforeman.org/t/drop-debian-11-ruby-2-7-and-nodejs-14-support-in-foreman-3-14/40503 first. Once we drop NodeJS 14 we should get NPM 8 or higher out of the box on all our supported platforms and that simplifies this PR.

Edit: for completeness, the NPM versions in packaging:

  • EL9 with NodeJS 18: 10.2.4 (dnf module enable nodejs:18 && dnf install npm)
  • Debian 12 with NodeJS 17: 9.2.0 (https://packages.debian.org/bookworm/npm)
  • Ubuntu 22.04: TODO (OS repo has NodeJS 12 so we use nodesource; 14 at the moment)

@ekohl
Copy link
Member

ekohl commented Dec 19, 2024

@theforeman/packaging are we ok to update to NPM 8+?

When we merge #10406 it should be something we get for free.

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.

2 participants