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

Improve CJS and ESM module support #1037

Closed
wants to merge 8 commits into from

Conversation

d98762625
Copy link
Member

Functional changes:

  • Can now use chef with CJS and ES Modules
  • use exports in package.json to explicitly define entrypoints for import and require routes
  • can now use named imports with NodeJS v12+

load via require
Loading via require will take you to the esm wrapped module which exports the top-level ESModule as a CJS module. This provides full compatibility with ESM modules, but loading takes a performance hit, as shown in this PR

Performance profile

console.time('require');
require('cyberchef')
console.timeEnd('require');

// => require: 6313.314ms

Screenshot 2020-05-22 at 15 52 57

load via import
Node.js as increasing support for ESM / CJS interoperability. We can use this with a couple of extra flags. A consumer will need to use --experimental-json-modules to load JSON files as modules, and --experimental-specifier-resolution to resolve non-explicit imports in our dependencies.

Performance profile

console.time('import');
import chef from "cyberchef";
console.timeEnd('import');

// => import: 0.110ms

Screenshot 2020-05-22 at 17 24 22

Later Node.js versions
I've tested this with latest stable (14.3.0) - you don't need the --experimental-modules flag but you do need the other two when using import.

@Prinzhorn
Copy link
Contributor

First of all thank you very much for looking into this.

I'm using node 12 and did this:

  1. I did a clean checkout of your branch
  2. npm i
  3. npm run build
Running "exec:generateConfig" (exec) task

--- Regenerating config files. ---
Written operation index.
--- Config scripts finished. ---

internal/modules/run_main.js:57
    internalBinding('errors').triggerUncaughtException(
                              ^

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".json" for /home/alex/Projects/Bandsalat/issues/cyberchef-import-maybefast/CyberChef/src/core/config/OperationConfig.json
    at Loader.defaultGetFormat [as _getFormat] (internal/modules/esm/get_format.js:70:15)
    at Loader.getFormat (internal/modules/esm/loader.js:110:42)
    at Loader.getModuleJob (internal/modules/esm/loader.js:241:31)
    at async ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:44:17)
    at async Promise.all (index 0)
    at async link (internal/modules/esm/module_job.js:48:9) {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}

I had to update the Gruntfile to make it run. Did I do something wrong or did this run through on your end?

diff --git a/Gruntfile.js b/Gruntfile.js
index 8814780e..443bf901 100755
--- a/Gruntfile.js
+++ b/Gruntfile.js
@@ -369,8 +369,8 @@ module.exports = function (grunt) {
                 command: chainCommands([
                     "echo '\n--- Regenerating config files. ---'",
                     "echo [] > src/core/config/OperationConfig.json",
-                    "node --experimental-modules --no-warnings --no-deprecation src/core/config/scripts/generateOpsIndex.mjs",
-                    "node --experimental-modules --no-warnings --no-deprecation src/core/config/scripts/generateConfig.mjs",
+                    "node --experimental-modules --experimental-json-modules --experimental-specifier-resolution=node --no-warnings --no-deprecation src/core/config/scripts/generateOpsIndex.mjs",
+                    "node --experimental-modules --experimental-json-modules --experimental-specifier-resolution=node --no-warnings --no-deprecation src/core/config/scripts/generateConfig.mjs",
                     "echo '--- Config scripts finished. ---\n'"
                 ]),
                 sync: true

I'll look into the actual performance stuff now. I don't think console.time works in this case, I assume the import is hoisted because 0.110ms is not realistic.

@Prinzhorn
Copy link
Contributor

Same when running ./node_modules/.bin/grunt node, the exec:generateNodeIndex fails. But I got it running now.

diff --git a/Gruntfile.js b/Gruntfile.js
index 8814780e..853e30fc 100755
--- a/Gruntfile.js
+++ b/Gruntfile.js
@@ -369,8 +369,8 @@ module.exports = function (grunt) {
                 command: chainCommands([
                     "echo '\n--- Regenerating config files. ---'",
                     "echo [] > src/core/config/OperationConfig.json",
-                    "node --experimental-modules --no-warnings --no-deprecation src/core/config/scripts/generateOpsIndex.mjs",
-                    "node --experimental-modules --no-warnings --no-deprecation src/core/config/scripts/generateConfig.mjs",
+                    "node --experimental-modules --experimental-json-modules --experimental-specifier-resolution=node --no-warnings --no-deprecation src/core/config/scripts/generateOpsIndex.mjs",
+                    "node --experimental-modules --experimental-json-modules --experimental-specifier-resolution=node --no-warnings --no-deprecation src/core/config/scripts/generateConfig.mjs",
                     "echo '--- Config scripts finished. ---\n'"
                 ]),
                 sync: true
@@ -378,7 +378,7 @@ module.exports = function (grunt) {
             generateNodeIndex: {
                 command: chainCommands([
                     "echo '\n--- Regenerating node index ---'",
-                    "node --experimental-modules --no-warnings --no-deprecation src/node/config/scripts/generateNodeIndex.mjs",
+                    "node --experimental-modules --experimental-json-modules --experimental-specifier-resolution=node --no-warnings --no-deprecation src/node/config/scripts/generateNodeIndex.mjs",
                     "echo '--- Node index generated. ---\n'"
                 ]),
                 sync: true

@Prinzhorn
Copy link
Contributor

So this definitely helped a lot, here's a more realistic test that shows a rough 50% improvement.

require.js

const chef = require('./CyberChef/src/node/wrapper.js');
console.log(chef.fromBase64("U28gbG9uZyBhbmQgdGhhbmtzIGZvciBhbGwgdGhlIGZpc2gu"));

import.mjs

import chef from './CyberChef/src/node/index.mjs';
console.log(chef.fromBase64("U28gbG9uZyBhbmQgdGhhbmtzIGZvciBhbGwgdGhlIGZpc2gu"));
$ time node require.js 
So long and thanks for all the fish.

real	0m3,437s
user	0m3,855s
sys	0m0,239s
$ time node --experimental-modules --experimental-json-modules --experimental-specifier-resolution=node import.mjs 
(node:15460) ExperimentalWarning: The ESM module loader is experimental.
(node:15460) ExperimentalWarning: Importing JSON modules is an experimental feature. This feature could change at any time
So long and thanks for all the fish.

real	0m1,572s
user	0m1,711s
sys	0m0,118s

Now let's see if I can compile CyberChef down to a single file using ncc, that would be amazing. Because it's still a lot of files that need to be loaded. I'll report back.

@Prinzhorn
Copy link
Contributor

For ncc (and node in general?) these Webpack specific parts are still a problem

const fonts = [
import(/* webpackMode: "eager" */ "../../web/static/fonts/bmfonts/Roboto72White.fnt"),
import(/* webpackMode: "eager" */ "../../web/static/fonts/bmfonts/RobotoBlack72White.fnt"),
import(/* webpackMode: "eager" */ "../../web/static/fonts/bmfonts/RobotoMono72White.fnt"),
import(/* webpackMode: "eager" */ "../../web/static/fonts/bmfonts/RobotoSlab72White.fnt")
];

@d98762625
Copy link
Member Author

@Prinzhorn I also ran into those issues (and so did our CI) so I've put those fixes onto this branch 👍

I have never tried to use ncc, please let us know if you find a solution.

Some UI-specific operations are bound to fail in the NodeJS API. I think we haven't come across them all yet because, like the example you've shown above, they'll only try and import those files when the function is run. Feel free to raise a separate issue for that.

Node does support dynamic import. As I said, if you do look into that issue, let us know what you find

@d98762625 d98762625 mentioned this pull request May 29, 2020
@jl5193 jl5193 mentioned this pull request Feb 4, 2022
n1474335 added a commit that referenced this pull request Mar 28, 2022
@n1474335
Copy link
Member

The changes in this branch have been merged in v9.33.0 #1326

@n1474335 n1474335 closed this Mar 28, 2022
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.

3 participants