Fix broken NPM bundle by publishing in ES-Module format #1944

elikoga · 2025-03-17T15:06:28Z

Closes #1943

Tells Babel that the target is ES Modules to workaround "Babel doesn't support transforming top-level await" (https://babeljs.io/docs/babel-plugin-syntax-top-level-await)
Tells Node that files are ES Modules (package.json: "type":"module")
Rename deprecated npm prepublish script to npm prepare (https://docs.npmjs.com/cli/v8/using-npm/scripts)
Renames already written CommonJS files to file ending ending with ".cjs"

This fixes the issue that the generated CommonJS NPM-Published Bundle contains invalid CommonJS (top level await is not supported in CommonJS), which makes tooling fail.

What is important to note is that a top-level await itself is not the problem at all, the only problem is the fact that the published bundle contains invalid CommonJS. The CommonJS format does not allow top-level awaits.

CI passes on my master.

CendioOssman

Thanks! Nice work!

I had a look at what this might require from users. And it sees Node.js 14 will be needed. But that is long EoL, so I would assume all users have a sufficiently new Node.js for this.

package.json

CendioOssman · 2025-03-19T08:40:30Z

package.json

-    "prepublish": "node ./utils/convert.js --clean"
+    "lint": "eslint app core po/po2js.cjs po/xgettext-html.cjs tests utils",
+    "test": "karma start karma.conf.cjs",
+    "prepare": "node ./utils/convert.cjs --clean"


Do we need a conversion step at all?

We might be able to get away with simply copying a folder, however that would increase the complexity of the patch.

There is little harm in having an additional no-op buildstep except for the build times.

I have modified convert.js to a build.js that copies the required js files to the target folder without using babel

Reverted: Actually, since we are using Babel to rewrite vendored imports, the conversion script is needed.

The rewriting does not seem to work properly. I'm not seeing any changes.

We could also consider dropping that as well, as the relative path will be the same.

In fact, is there any requirement on a lib/ directory? We mostly had that since the files were change. Is there anything preventing us from just shipping core/ and vendor/ as is?

I don't see any fundamental problem, except for the fact that I use the import as follows let RFB = await import('@novnc/novnc/lib/rfb.js'); so I would want to preserve some file-copying so that we keep the imports the same as before.

Cockpit uses the same import path as far as I can see:
https://github.com/cockpit-project/cockpit-machines/blob/d0a26ccf69f1969e02ef4a0c8e835da305dd43f9/src/components/vm/consoles/consoles.jsx#L22C34-L22C58
https://github.com/patternfly/react-console/blob/1cf89cb13e829f3691010687ea034f90b59f3cf7/packages/module/src/components/VncConsole/VncConsole.tsx#L7

We specify the primary import in package.json. What is that used for? Can't users of our npm package respect that somehow?

https://docs.npmjs.com/cli/v11/configuring-npm/package-json#browser

If your module is meant to be used client-side the browser field should be used instead of the main field. This is helpful to hint users that it might rely on primitives that aren't available in Node.js modules. (e.g. window)

It seems to be cosmetic/for documentation purposes but I am still testing some stuff

My tooling gives me:

[plugin:vite:import-analysis] Failed to resolve entry for package "@novnc/novnc". The package may have incorrect main/module/exports specified in its package.json.

So the value seems to be ignored by my flavour of tooling

A modern solution would be to use the exports field:

https://docs.npmjs.com/cli/v11/configuring-npm/package-json#exports
https://nodejs.org/api/packages.html#package-entry-points

Which is supported since Node 12 and would allow us to make the npm-published slimmer while keeping old imports valid.

…n variable

elikoga · 2025-03-25T12:23:33Z

Rebased on to master

CendioOssman · 2025-03-28T12:28:22Z

eslint.config.mjs

@@ -62,7 +62,7 @@ export default [
        }
    },
    {
-        files: ["po/po2js", "po/xgettext-html"],
+        files: ["po/po2js.js", "po/xgettext-html.js"],


There doesn't seem to be any need to add any extension to these commands now?

No need, can change back

CendioOssman · 2025-03-28T12:35:46Z

package.json

-    "prepublish": "node ./utils/convert.js --clean"
+    "lint": "eslint app core po/po2js.js po/xgettext-html.js tests utils",
+    "test": "karma start karma.conf.cjs",
+    "prepare": "node ./utils/convert.js --clean"


For clarity, the change to "prepare" is best kept in its own commit.

CendioOssman · 2025-03-28T12:47:57Z

package.json

-    "prepublish": "node ./utils/convert.js --clean"
+    "lint": "eslint app core po/po2js.cjs po/xgettext-html.cjs tests utils",
+    "test": "karma start karma.conf.cjs",
+    "prepare": "node ./utils/convert.cjs --clean"


The rewriting does not seem to work properly. I'm not seeing any changes.

We could also consider dropping that as well, as the relative path will be the same.

In fact, is there any requirement on a lib/ directory? We mostly had that since the files were change. Is there anything preventing us from just shipping core/ and vendor/ as is?

CendioOssman · 2025-03-28T12:48:48Z

utils/convert.js

        presets: [
            [ '@babel/preset-env',
-              { modules: 'commonjs' } ]
+              { modules: false } ]
        ],


The presets is doing some weird stuff with the code. We likely want to drop this (if we don't end up dropping babel completely).

I can try dropping babel once again

elikoga mentioned this pull request Mar 17, 2025

noVNC cannot be bundled due to top-level await in browser.js #1943

Open

elikoga force-pushed the master branch 2 times, most recently from 88f0580 to b5be498 Compare March 17, 2025 15:48

elikoga changed the title ~~Fix NPM bundle by publishing in ES-Module format~~ Fix broken NPM bundle by publishing in ES-Module format Mar 17, 2025

consideRatio mentioned this pull request Mar 18, 2025

Failure to build project's JS based on error in novnc 1.6.0 released March 12 jupyterhub/jupyter-remote-desktop-proxy#141

Open

CendioOssman requested changes Mar 19, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

CendioOssman requested changes Mar 19, 2025

View reviewed changes

elikoga force-pushed the master branch from 5e33d53 to 1235786 Compare March 20, 2025 16:12

elikoga requested a review from CendioOssman March 24, 2025 22:51

elikoga added 8 commits March 25, 2025 13:22

Refactor project to use ES modules and update related scripts

6220102

Migrate scripts to ES modules and update related configurations

27973d9

Add gettext .po to noVNC .js converter script

b013e29

Fix for loop declaration in xgettext-html.js to use 'let' for locatio…

892760b

…n variable

Fix eslint lint error

687444e

Replace build script with convert script for module transformation

57c0332

Update Babel preset configuration to use ES modules

aebb1ae

Add support for ES module path resolution in convert.js

Loading
Loading status checks…

1dc4cdf

elikoga force-pushed the master branch from 1235786 to 1dc4cdf Compare March 25, 2025 12:22

CendioOssman requested changes Mar 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix broken NPM bundle by publishing in ES-Module format #1944

Fix broken NPM bundle by publishing in ES-Module format #1944

elikoga commented Mar 17, 2025 •

edited

Loading

CendioOssman left a comment

CendioOssman Mar 19, 2025

elikoga Mar 19, 2025

elikoga Mar 20, 2025

elikoga Mar 20, 2025

CendioOssman Mar 28, 2025

elikoga Mar 28, 2025

CendioOssman Mar 28, 2025

elikoga Mar 28, 2025 •

edited

Loading

elikoga Mar 28, 2025

elikoga Mar 28, 2025

elikoga commented Mar 25, 2025

CendioOssman Mar 28, 2025

elikoga Mar 28, 2025

CendioOssman Mar 28, 2025

CendioOssman Mar 28, 2025

CendioOssman Mar 28, 2025

elikoga Mar 28, 2025

Fix broken NPM bundle by publishing in ES-Module format #1944

Are you sure you want to change the base?

Fix broken NPM bundle by publishing in ES-Module format #1944

Conversation

elikoga commented Mar 17, 2025 • edited Loading

CendioOssman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elikoga Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elikoga commented Mar 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elikoga commented Mar 17, 2025 •

edited

Loading

elikoga Mar 28, 2025 •

edited

Loading