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

Move from webpack to esbuild bundler #636

Merged
merged 4 commits into from
Apr 12, 2023

Conversation

subhoghoshX
Copy link
Member

Sync build.js with cockpit-podman and cockpit-machines.

Things I couldn't do and need assistance with:

  1. Changing the COCKPIT_REPO_COMMIT hash (https://github.com/cockpit-project/starter-kit/blob/main/Makefile#L37)
  2. Updating node_modules

Things I did differently:

  1. Change sourcemap from external to linked. It fixes the issue of sourcemap not working. (https://esbuild.github.io/api/#sourcemap)
  2. Update thenotify-end esbuild plugin to also print the time it takes to finish building.
    image

@martinpitt
Copy link
Member

Thanks for looking into this!

  1. You shouldn't need to update COCKPIT_REPO_COMMIT -- I already did that in PR Bump Cockpit API to 288.1 + esbuild plugin updates, fix TF failures #632 to prepare for moving to esbuild.
  2. starter-kit does not use a git repo for node_modules/, it's the classic npm install. So no magic necessary for this. It's unsafe for the developer and much slower, but as starter-kit is a template repo, we don't want to break the "just works" with "you have to do this complicated setup first".

The tests fail because this breaks translations. There's something wrong either with building dist/po.de.js, or with including it into the HTML page. Would you like to debug this yourself, for the learning exercise, and ask questions here or on Matrix, or would you like me to debug this myself?

@martinpitt martinpitt marked this pull request as draft April 11, 2023 08:17
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

@subhoghoshX po has to be loaded after index.js in order that it has the global cockpit object available. See cockpit-project/cockpit-podman@cbdd2a3

You can also remove the base1/cockpit.js include in the html file, as this is not external any more.

diff --git src/index.html src/index.html
index 3845f06..6eb0369 100644
--- src/index.html
+++ src/index.html
@@ -24,9 +24,8 @@ along with this package; If not, see <http://www.gnu.org/licenses/>.
 
     <link rel="stylesheet" href="index.css">
 
-    <script type="text/javascript" src="../base1/cockpit.js"></script>
-    <script type="text/javascript" src="po.js"></script>
     <script type="text/javascript" src="index.js"></script>
+    <script type="text/javascript" src="po.js"></script>
 </head>
 
 <body class="pf-m-redhat-font">

@subhoghoshX
Copy link
Member Author

All TF tests passed! (except aarch64 ones). Thanks @KKoukiou :)

@martinpitt
Copy link
Member

Indeed this fails to build on non-x86_64 as there is no esbuild-wasm fallback. We have that in cockpit.git, but not yet in c-podman/machines (we still need to do that).

@subhoghoshX
Copy link
Member Author

Indeed this fails to build on non-x86_64 as there is no esbuild-wasm fallback. We have that in cockpit.git, but not yet in c-podman/machines (we still need to do that).

Ah yes. That makes sense. May I give it a shot after this?

@martinpitt
Copy link
Member

@subhoghoshX : yes, of course! Let's add esbuild-wasm here (taking it from cockpit), and once it lands, please add it to these others. It doesn't block podman/machines as these don't rebuild the bundle during rpm build, but starter-kit and cockpit-certificates do.

@subhoghoshX
Copy link
Member Author

Yay, all tests PASSED! I figured I don't need to do any of the complicated things from Cockpit build.js as it has only one entry point 😄

@KKoukiou KKoukiou marked this pull request as ready for review April 11, 2023 17:37
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

👏 excellent work! I'm impressed how fast you picked this stuff up!

Let's do some cleanups, and land this.

build.js Show resolved Hide resolved
build.js Show resolved Hide resolved
build.js Outdated Show resolved Hide resolved
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Eeeexcellent, thank you! I triggered our integration tests.

@martinpitt
Copy link
Member

@subhoghoshX Feel free to propose your improvements (build time, sourcemap, wasm) to cockpit-machines/podman, if you like. Cheers!

@martinpitt martinpitt merged commit 1c30b49 into cockpit-project:main Apr 12, 2023
10 checks passed
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.

None yet

3 participants