hawtio / hawtio-online

Hawtio on Kubernetes/OpenShift
Apache License 2.0
24 stars 25 forks source link

fix: Use webpack DefinePlugin to inject the package version #460

Closed phantomjinx closed 1 month ago

phantomjinx commented 1 month ago
tadayosi commented 1 month ago

Indeed, if you just run the following without replace-version at online-shell:

$ yarn workspace @hawtio/online-shell build:webpack
$ ag __HAWTIO_ONLINE_PACKAGE_VERSION_PLACEHOLDER__ packages/online-shell/build/ -l
packages/online-shell/build/static/js/144.chunk.js
packages/online-shell/build/static/js/144.chunk.js.map

it's still reserved in a chunk. And replace-version is indeed working:

$ yarn workspace @hawtio/online-shell replace-version
build/static/js/144.chunk.js
 2: "use strict";(self.webpackChunk_hawtio_online_shell=self.webpackChunk_hawtio_online_shell||[]).push([[144],{29987:(e,n,o)=>{o.d(n,{A:()=>i});var t=o(11182),r=o.n(t),a=o(32806),s=o.n(a)()(r());s.push([e.id,".console-link {\n  margin-left: 0.3em !important;\n  margin-right: 0.3em !important;\n  margin-top: 0.2em !important;\n  margin-bottom: 0.2em !important;\n}\n\n.console-link:hover {\n  text-deco
build/static/js/144.chunk.js.map
 1: {"version":3,"file":"static/js/144.chunk.js","mappings":";gLAGIA,QAA0B,GAA4B,KAE1DA,EAAwBC,KAAK,CAACC,EAAOC,GAAI,kOAUtC,GAAG,CAAC,QAAU,EAAE,QAAU,CAAC,uCAAuC,MAAQ,GAAG,SAAW,wFAAwF,eAAiB,CAAC,mOAAmO,WAAa,MAErb,8ECdIH,QAA0B,GAA4B,KAE1DA,EAAwBC,KAAK,CAACC,EAAOC,GAAI,mgEAkHtC,GAAG,CAAC,QAAU,EAAE,QAAU,CAAC,yCAAyC,MAAQ,GAAG,SAAW,i0BAAi0B,eAAiB,CAAC,ogEAAogE,WAAa,MAEj8F,8ECtHIH,QAA0B,GAA4B,KAE1DA,EAAwBC,K
$ ag "Hawtio Online.+2.0.0" packages/online-shell/build/static/js/144.chunk.js
...i.configManager.addProductInfo("Hawtio Online","2.0.0");...

So I suspect some other place in the whole build process has a problem instead of the replace-version script.

tadayosi commented 1 month ago

So the issue seems to be __PACKAGE_VERSION_PLACEHOLDER__ rather than __HAWTIO_ONLINE_PACKAGE_VERSION_PLACEHOLDER__. __PACKAGE_VERSION_PLACEHOLDER__ is used in other internal packages like k8s-api and mgmt-api. It shouldn't be the issue of replace-version at online-shell but in the way it bundle all packages together but without correctly updating __PACKAGE_VERSION_PLACEHOLDER__ at each sub package.

phantomjinx commented 1 month ago

So the issue seems to be __PACKAGE_VERSION_PLACEHOLDER__ rather than __HAWTIO_ONLINE_PACKAGE_VERSION_PLACEHOLDER__. __PACKAGE_VERSION_PLACEHOLDER__ is used in other internal packages like k8s-api and mgmt-api. It shouldn't be the issue of replace-version at online-shell but in the way it bundle all packages together but without correctly updating __PACKAGE_VERSION_PLACEHOLDER__ at each sub package.

No. This is not the issue as I changed this only yesterday fearing that the replace script might be attempting to overwrite the PACKAGE_VERSION_PLACEHOLDER in the @hawtio/react library as well as the hawtio-online.

[: … node_modules] version-constant+* ± cd node_modules/@hawtio/react
[: … react] version-constant+* ± ls
CHANGELOG.md  dist  LICENSE  node_modules  package.json  README.md
[: … react] version-constant+* ± cd dist
[: … dist] version-constant+* ± ls
index.css  index.css.map  index.d.ts  index.js  index.js.map
[: … dist] version-constant+* ± grep -r PACKAGE_ *
... <lots of text>
uctInfo('Hawtio React', '__PACKAGE_VERSION_PLACEHOLDER__')\n","export * from './config-manager'\nexport * from './core'\nexport * from './event-service'\nexport * from './hooks'\nexport * from './logging'\n","import $ from 'jquery'\nimport { Plugin } from './core'\nimport { Logger } from './logging'\n\nconst log = Logger.get('hawtio-c
... <lots more text>

Given that the @hawtio/react version is correctly displayed in the About box, this particular __PACKAGE_VERSION_PLACEHOLDER, ~I think,~ is the one from the relevant source map.

phantomjinx commented 1 month ago

The files that from Hawtio-Online build that contain 'addProductInfo('

and yet bringing up the build in a container: Screenshot_20240510_115801

So webpack must have included the PLACEHOLDER value somewhere else in the code bundles that has already been uglified so replace-version cannot find it. Using webpack DefinePlugin ensures that the value is correctly substitued during webpack's compilation routine, updating the chunk.js and the About panel:

Screenshot_20240510_120900

Generally speaking, patching randomly without capturing the root causes doesn't improve code quality overtime.

This was not a random patch (or hack) but the result of research into a valid method for the webpack framework for injecting a variable value into code at webpack compile time.

tadayosi commented 1 month ago

and yet bringing up the build in a container:

Why? That's what I'm asking. If all the __HAWTIO_ONLINE_PACKAGE_VERSION_PLACEHOLDER__ are already replaced in the build, why is it still displayed in the final product?

So webpack must have included the PLACEHOLDER value somewhere else in the code bundles that has already been uglified so replace-version cannot find it.

Can you elaborate more what that really means? What's exactly the case where replace cannot catch up in the uglified output?

phantomjinx commented 1 month ago

and yet bringing up the build in a container:

Why? That's what I'm asking. If all the __HAWTIO_ONLINE_PACKAGE_VERSION_PLACEHOLDER__ are already replaced in the build, why is it still displayed in the final product?

So webpack must have included the PLACEHOLDER value somewhere else in the code bundles that has already been uglified so replace-version cannot find it.

Can you elaborate more what that really means? What's exactly the case where replace cannot catch up in the uglified output?

I think rather that uglified, it is in fact compression. For every js and js.map file created a gz archive is also created. Cannot find it yet but the fact the CompressionPlugin is included would imply those chuck.gz files are used rather than js files?? Of course the replace script will not be able to find the PLACEHOLDER and replace it.

[: … js] 2.x-redhat+* ± mkdir triage
[: … js] 2.x-redhat+* ± ls
144.chunk.js                 475.chunk.js.gz              564.chunk.js.map.gz          891.chunk.js
144.chunk.js.gz              475.chunk.js.LICENSE.txt     601.chunk.js                 891.chunk.js.gz
144.chunk.js.LICENSE.txt     475.chunk.js.map             601.chunk.js.gz              891.chunk.js.LICENSE.txt
144.chunk.js.map             475.chunk.js.map.gz          601.chunk.js.LICENSE.txt     891.chunk.js.map
144.chunk.js.map.gz          485.chunk.js                 601.chunk.js.LICENSE.txt.gz  891.chunk.js.map.gz
...

[: … js] 2.x-redhat+* ± cp 144*.gz triage
[: … js] 2.x-redhat+* ± cd triage/
[: … triage] 2.x-redhat+* ± ls
144.chunk.js.gz  144.chunk.js.map.gz

[: … triage] 2.x-redhat+* ± gzip -d 144.chunk.js.gz
[: … triage] 2.x-redhat+* ± ls
144.chunk.js  144.chunk.js.map.gz

[: … triage] 2.x-redhat+* ± grep addProductInfo\( 144.chunk.js
...
i.configManager.addProductInfo("Hawtio Online","__HAWTIO_ONLINE_PACKAGE_VERSION_PLACEHOLDER__"
...
tadayosi commented 1 month ago

I think rather that uglified, it is in fact compression. For every js and js.map file created a gz archive is also created. Cannot find it yet but the fact the CompressionPlugin is included would imply those chuck.gz files are used rather than js files?? Of course the replace script will not be able to find the PLACEHOLDER and replace it.

That makes sense. I'd still try to apply replace-version before compressing it by the plugin, but at least your changes make sense given the explanation.

phantomjinx commented 1 month ago

I cannot see a webpack plugin that would allow me to continue to use replace as-is. There is this one but not recommended for production.

Other thought was running replace prior to the build but that would mean replacing in the src which is not ideal as that could be accidentally committed.