Skip to content

Conversation

@grgrzybek
Copy link
Contributor

I know this PR is big. Not only it merges with #109 from @ViliusS, but fixes several things at once...

I wasn't able to send a PR for each separate JIRA and issue I worked on. The reason is that some changes needed to be added together.

Here's a list of fixed issues:

  • update to @hawtio/react 1.10.1
  • separation of initialization and UI code (because of Hawtio update)
  • changes to normal/dev/peer dependencies according to recent findings and work in Hawtio
  • fixes to D3/WebCola layout
  • fixes to uncaught promise issue related to webback-dev-server overlays
  • usage of individual @patternfly/react-icons (to shrink bundle size)
  • dynamic Hawtio plugin registration according to new docs: https://hawt.io/docs/developers/applications.html

Comment on lines 13 to 23
"@hawtio/react": "^1.10.1",
"@monaco-editor/react": "^4.6.0",
"@patternfly/react-charts": "^7.4.9",
"@patternfly/react-code-editor": "^5.4.18",
"@patternfly/react-core": "^5.4.14",
"@patternfly/react-icons": "^5.4.2",
"@patternfly/react-styles": "^5.4.1",
"@patternfly/react-table": "^5.4.16",
"@patternfly/react-tokens": "^5.4.1",
"@patternfly/react-topology": "^5.4.1",
"artemis-console-plugin": "workspace:*",
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this against what is written in https://github.com/hawtio/hawtio-next/tree/main/app#dependencies-dev-dependencies-peer-dependencies, i.e. "... app should declare a dependency only if it is used directly in its code"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The app is using very little, but it uses @hawtio/react and artemis-console-plugin which use peer dependencies. So the app has to satisfy these peer requirements.

I'm not claiming my guide from Hawtio is written in stone ;) I probably need to revisit it from time to time.

"webpack": "^5.99.0",
"webpack-cli": "^5.1.4",
"webpack-manifest-plugin": "^5.0.0"
"webpack": "^5.101.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm missing something, webpack is only used by the app, so should not be needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The webpack issue here is special and tricky. @hawtio/react is using @module-federation/utilities, which has unnecessary (IMO) peer dependency on webpack.

At top level I have fixed it in .yarnrc.yml:

  # this one is strange:
  #  - there's peer dependency to webpack, but I don't think it's necessary
  #  - suggested @module-federation/runtime doesn't refer to variables like __webpack_require__, so it doesn't look
  #    like direct replacement of @module-federation/utilities
  '@module-federation/utilities@*':
    peerDependenciesMeta:
      'webpack':
        'optional': true

Copy link
Contributor

Choose a reason for hiding this comment

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

But webpack version is specified as a devDependency in both the package and the app - isn't it enough to specify it as a peerDependency in package, and only specify devDependency in app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the rule that if something is a peer dependency it probably should also be a dev dependency (so yarn install installs it, because it may be needed).

  • in package: webpack is a dev dependency because it's peer dependency (for @module-federation/utilities to propagate its peer dependency on webpack - even if it's made optional in .yarnrc.yml)
  • in app: webpack is a dev dependency because it's used as a bundler.

I know it may be confusing, it is for me, it's not what I'd expect, but I'm still learning ;)

@GChuf
Copy link
Contributor

GChuf commented Jul 31, 2025

Tested dev and prod build (yarn start and mvn run jetty) ... I have errors on both. I did delete node modules and ran yarn install again.
This is the result for jetty:
image

@grgrzybek do you have any ideas? If not I can try to track it down to a particular commit if i'll have the time.
p.s. that's assuming the changes are the problem, and not something in my environment ...

@grgrzybek
Copy link
Contributor Author

@GChuf for shared module federation config from webpack.config.js:

new ModuleFederationPlugin({
  shared: {
    'react': {
      singleton: true,
      requiredVersion: dependencies['react'],
    },
    'react-dom': {
      singleton: true,
      requiredVersion: dependencies['react-dom'],
    },
    'react-router-dom': {
      singleton: true,
      requiredVersion: dependencies['react-router-dom'],
    },
    '@hawtio/react': {
      singleton: true,
      requiredVersion: dependencies['@hawtio/react'],
    },
    'monaco-editor': {
      singleton: true,
      requiredVersion: dependencies['monaco-editor'],
    },
    '@patternfly/react-core': {
      singleton: true,
      requiredVersion: dependencies['@patternfly/react-core'],
    },
  }
}),

the generated code (with yarn webpack --mode development) in main.bundle.js should be:

switch(name) {
	case "default": {
		register("@hawtio/react", "1.10.1", () => (Promise.all([__webpack_r ...
		register("@patternfly/react-core", "5.4.14", () => (Promise.all([__ ...
		register("monaco-editor", "0.52.2", () => (Promise.all([__webpack_r ...
		register("react-dom", "18.3.1", () => (Promise.all([__webpack_requi ...
		register("react-router-dom", "6.30.1", () => (Promise.all([__webpac ...
		register("react", "18.3.1", () => (__webpack_require__.e("vendors-n ...
	}
	break;
}

the production chunk (in my case runtime.edc42c83.js) contains:

  y = {
    16828: () => g("default", "react", !1, [ 1, 18, 3, 1 ], () => j.e(644).then(() => () => j(63696))),
    89144: () => g("default", "react-dom", !1, [ 1, 18, 3, 1 ], () => Promise.all([ j.e(644), j.e(695) ]).then(() => () => j(78325))),
    1728: () => g("default", "monaco-editor", !1, [ 2, 0, 52, 2 ], () => Promise.all([ j.e(502), j.e(859) ]).then(() => () => j(11918))),
    40266: () => g("default", "@patternfly/react-core", !1, [ 1, 5, 4, 14 ], () => Promise.all([ j.e(176), j.e(612), j.e(806), j.e(695) ]).then(() => () => j(73196))),
    66418: () => g("default", "react-router-dom", !1, [ 1, 6, 30, 1 ], () => Promise.all([ j.e(644), j.e(695) ]).then(() => () => j(49348))),
    4201: () => g("default", "@hawtio/react", !1, [ 1, 1, 10, 1 ], () => Promise.all([ j.e(176), j.e(612), j.e(806), j.e(631), j.e(695), j.e(680) ]).then(() => () => j(20596)))
  },

where [ 1, 5, 4, 14 ] matches "5.4.14".

What you see (Unsatisfied version 6.2.0) definitely comes from latest @patternfly/react-core version which we don't use...

Let me check with jetty:run.

@grgrzybek
Copy link
Contributor Author

Works for me:
image

image

@GChuf
Copy link
Contributor

GChuf commented Jul 31, 2025

Uh, im having mixed results still.
I cannot reproduce the previous error, but I have other errors now (in development mode)

After user logs in and tries to connect to artemis, I get

Uncaught runtime errors:
ERROR
Unknown promise rejection reason
handleError@http://localhost:8080/hawtio/static/js/main.bundle.js:3174:58
../node_modules/webpack-dev-server/client/overlay.js/createOverlay/<@http://localhost:8080/hawtio/static/js/main.bundle.js:3202:18
EventListener.handleEvent*listenToUnhandledRejection@http://localhost:8080/hawtio/static/js/main.bundle.js:2866:10
createOverlay@http://localhost:8080/hawtio/static/js/main.bundle.js:3200:31
../node_modules/webpack-dev-server/client/index.js?protocol=ws%3A&hostname=0.0.0.0&port=8080&pathname=%2Fws&logging=info&overlay=true&reconnect=10&hot=true&live-reload=true@http://localhost:8080/hawtio/static/js/main.bundle.js:1358:105
__webpack_require__@http://localhost:8080/hawtio/static/js/main.bundle.js:3879:32
@http://localhost:8080/hawtio/static/js/main.bundle.js:5467:30
@http://localhost:8080/hawtio/static/js/main.bundle.js:5471:12

After that, I click the X button on top right and it redirects me to artemis login (I basically login 2 times)

I had another error which I also cannot reproduce... i might be going insane already

But I can see this in dev mode, and im so happy dark mode is now an option:
image

@GChuf
Copy link
Contributor

GChuf commented Jul 31, 2025

I can confirm jetty:run also works.

I think the issues im seeing are only there on development builds.
I tested dev and prod builds through jetty.

Development builds only has the following errors:

before "connecting", so right after inputting the URL into browser:

ERROR
javax.management.InstanceNotFoundException : *:*
    at handleError (http://localhost:8080/console/static/js/main.bundle.js:3174:58)
    at http://localhost:8080/console/static/js/main.bundle.js:3202:7

After clicking connect and before logging in:

ERROR
Unknown promise rejection reason
    at handleError (http://localhost:8080/console/static/js/main.bundle.js:3174:58)
    at http://localhost:8080/console/static/js/main.bundle.js:3202:7

I was now able to reproduce this scenario 4 times.

@grgrzybek
Copy link
Contributor Author

The uncaught promise issue was supposed to be fixed with grgrzybek@b139a3a ...

let's move to desperate stage - did you check with private mode?

@GChuf
Copy link
Contributor

GChuf commented Jul 31, 2025

The uncaught promise issue was supposed to be fixed with grgrzybek@b139a3a ...

let's move to desperate stage - did you check with private mode?

I think you were right ... I started another instance on port 8081 and opened chrome's incognito mode - no errors.
Tested another browser on port 8081 (no cache because it's a "new" port?) and i also was not able to reproduce.

Testing frontends is a nightmare.

@GChuf
Copy link
Contributor

GChuf commented Jul 31, 2025

I've tested 6 browsers and all gave me the same result, I don't think the website was cached on all of them, but who knows ...

In any case I've been thinking of trying to develop a CICD for the console and host it somewhere. It should make developing/testing easier. It's obviously impossible to make it for every developer's branch, but we could make the cicd for main, and maybe for another "development" branch here on this repository.

@grgrzybek
Copy link
Contributor Author

I was never good at setting up CI/CD... But good idea!

@andytaylor
Copy link
Contributor

I cant get it to build, i get:

[INFO] --- frontend:1.15.1:corepack (yarn build) @ artemis-console-extension ---
[INFO] Running 'corepack yarn build' in /home/ataylor/IdeaProjects/activemq-artemis-console/artemis-console-extension/artemis-extension
[INFO] CLI Building entry: src/index.ts
[INFO] CLI Using tsconfig: tsconfig.json
[INFO] CLI tsup v8.5.0
[INFO] CLI Using tsup config: /home/ataylor/IdeaProjects/activemq-artemis-console/artemis-console-extension/artemis-extension/packages/artemis-console-plugin/tsup.config.ts
[INFO] CLI Target: esnext
[INFO] CLI Cleaning output folder
[INFO] CJS Build start
[INFO] CJS dist/index.css 1.28 KB
[INFO] CJS dist/index.js 284.42 KB
[INFO] CJS dist/index.css.map 4.39 KB
[INFO] CJS dist/index.js.map 398.39 KB
[INFO] CJS ⚡️ Build success in 99ms
[INFO] DTS Build start
[INFO] src/index.ts(50,10): error TS2339: Property 'addDeferredPlugin' does not exist on type 'HawtioCore'.
[INFO]
[INFO] Error: error occurred in dts build
[INFO] at Worker. (/home/ataylor/IdeaProjects/activemq-artemis-console/artemis-console-extension/artemis-extension/node_modules/tsup/dist/index.js:1545:26)
[INFO] at Worker.emit (node:events:518:28)
[INFO] at MessagePort. (node:internal/worker:268:53)
[INFO] at [nodejs.internal.kHybridDispatch] (node:internal/event_target:827:20)
[INFO] at MessagePort. (node:internal/per_context/messageport:23:28)
[INFO] DTS Build error
[INFO] The command failed in workspace artemis-console-plugin@workspace:packages/artemis-console-plugin with exit code 1
[INFO] The command failed for workspaces that are depended upon by other workspaces; can't satisfy the dependency graph
[INFO] Failed with errors in 4s 933ms

@GChuf
Copy link
Contributor

GChuf commented Aug 18, 2025

@andytaylor did you try deleting all node_modules folders before installing packages again?

@andytaylor
Copy link
Contributor

@GChuf thanks, that did the trick, I forgot there are now multiple node_modules, I will review and merge

@andytaylor
Copy link
Contributor

I have run thru and tested these changes and all look good, It is a big PR so if anything needs addressing we should raise a jira and fix separately.

I will check the licenses and update in a new PR if needed

@andytaylor
Copy link
Contributor

actually, @grgrzybek could you fix up the conflicts please

@grgrzybek
Copy link
Contributor Author

there are now multiple node_modules

I ensured that there should be only one top-level node_modules/...

@gemmellr
Copy link
Member

The reason this is saying it cant be rebased and merged due to conflicts seems to be due to the merge commit in the middle with the changes from #109, which were not linear with the other changes in the branch for this PR, and so the two streams of changes conflict with each other if trying to rebase the commits together.

I think it would be nice to get rid of those seeming 'work in progress' commits without Jira refs to also get rid of the non-linearity, and so also fix the 'has conflicts' problem with this PR, by combining the changes from 109 into 1 commit and fixing up the other commits accordingly (or reworking the individual commits, including adding a Jira reference, if keeping them separate is actually desirable).

@GChuf
Copy link
Contributor

GChuf commented Aug 21, 2025

Just adding my experience - I was able to merge grgrzybek:ggrzybek-console-fixes into main without any problems/conflicts through git cli interface. Github also seems to allow me to merge that branch into main in my own cloned repo through GUI interface, but not here in the official repo - by that I mean that official repo says there are conflicts and that the PR cannot be merged cleanly (I didn't dig deeper).

Is merging via CLI and pushing to main an option, or does it have to be done through github's merge PR process?

@gemmellr
Copy link
Member

Yes so was I before commenting. The repo is set up for rebase-and-merge which is likely the difference between here and your fork, and certainly the difference from a local merge.

Personally I think the commits are a bit of mess the way they are currently, especially those without references, and I would update them as I said. The changes could be pushed manually as they are if someone wants to.

@grgrzybek grgrzybek force-pushed the ggrzybek-console-fixes branch from 0e5d1d6 to ee78d44 Compare August 21, 2025 09:59
@GChuf
Copy link
Contributor

GChuf commented Aug 21, 2025

I see - I didn't know about rebase-and-merge setting, thanks for explaining.
I see @grgrzybek just force pushed a rebased? version :)

@andytaylor andytaylor merged commit 5c0813e into apache:main Aug 21, 2025
2 checks passed
@grgrzybek grgrzybek deleted the ggrzybek-console-fixes branch August 21, 2025 10:27
@grgrzybek
Copy link
Contributor Author

@GChuf yes ;) I hope for nice, old-school merge (a commit with two parents). I wanted to preserve commits from #109.

But finally I simply rebased everything in correct order (#109, #110 and my today's updates)

@ViliusS
Copy link
Contributor

ViliusS commented Aug 21, 2025

Just for future reference. An amazing write up on commit squash/rebase/merge best practices is on GitHub https://github.blog/developer-skills/github/write-better-commits-build-better-projects/ , In this case I would have squashed my commits into 1 or 2, as there is no reason to keep all (my) development mistakes in final history.

Anyway, I'm glad this is merged now. Kudos to everyone involved!

@grgrzybek
Copy link
Contributor Author

Thanks for the article @ViliusS !
I personally work a lot with legacy code, where git blame reaches back to 2007 or similar. Squashed commits with tabs→spaces and refactorings are really painful. But I understand that squashing may be good. It's a matter of intuition sometimes.

This PR was quite disruptive so I wanted to record my mistakes ;)

And btw, I'd love to get a T-Shirt with "I resolved yarn.lock conflicts and survived".

@GChuf
Copy link
Contributor

GChuf commented Aug 22, 2025

It might be a bit messy but I'm really glad that this is finally merged, and that no casualties were reported due to yarn.lock files :)

@grgrzybek
Copy link
Contributor Author

btw, I've just found the words to explain my behavior:

you can always squash later to rewrite history. You can never unsquash the commit.

@ViliusS
Copy link
Contributor

ViliusS commented Aug 23, 2025

Thanks for the article @ViliusS ! I personally work a lot with legacy code, where git blame reaches back to 2007 or similar. Squashed commits with tabs→spaces and refactorings are really painful. But I understand that squashing may be good. It's a matter of intuition sometimes.

I completely understand what you mean. I've bean working on various FoxPro and PHP code bases reaching 1996 myself :) That's why the article clearly implies that tabs->spaces and refractoring commits should go into separate commits. Then you can ignore those commits with blame ignore-revs https://docs.github.com/en/repositories/working-with-files/using-files/viewing-and-understanding-files#ignore-commits-in-the-blame-view

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants