Use import/export instead of require()/module.exports #5632

mrxz · 2025-01-17T17:07:12Z

Description:
As discussed in #5363 replacing CommonJS (require()/module.exports) with ES Modules (import/export) is the first step. This PR is a rebase of an earlier experiment I did, which replaces most usages.

A couple of notes:

No build-step would still require additional work, as some dependencies aren't ESM
ESLint requires the ecmaVersion to be 6 or higher, but this also allows all of the ES6 (2015) syntax and constructs. There doesn't seem to be a way to only allow import/export. Though it might be time to allow ES6 in the code base(?)
Some import statements could be refined, e.g. import * as X from 'x.js'; followed by var y = X.y can be simplified to import { y } from 'x.js';. This is done in some places, but not all. Generally the utils is being imported whole in a lot of places, where importing specific utilities would suffice. Generally preferable to only import what is needed to aid three-shaking (down the line).

I'll place some comments on this PR to highlight some significant changes.

Changes proposed:

Use import/export instead of require()/module.exports

mrxz · 2025-01-17T17:08:08Z

.eslintrc.json

  },
  "rules": {
+    /* To work towards no-build. */
+    "import/extensions": ["warn", "always", { "js": "always" }],


Webpack doesn't need the .js suffix, but if the goal is to work towards a no-build setup it will be required. Thought it would be a good idea to start enforcing it regardless.

mrxz · 2025-01-17T17:09:54Z

.eslintrc.json

+    {
+      "files": ["./src/utils/index.js"],
+      "parserOptions": {
+        "ecmaVersion": 11


Currently needed due to export as usage. The utils where the hardest part to migrate as they have very inconsistent usages of default exports and re-exports. Preferably these are streamlined and simplified, along with having consumers import only the utils they use.

mrxz · 2025-01-17T17:11:32Z

src/components/index.js

+import './meta-touch-controls.js';
+import './obb-collider.js';
+import './obj-model.js';
+import './oculus-go-controls.js';


Diff looks a tad strange, as I moved meta-touch-controls.js up to maintain alphabetical order.

mrxz · 2025-01-17T17:15:51Z

src/components/scene/inspector.js

-var utils = require('../../utils/');
+import { AFRAME_INJECTED } from '../../constants/index.js';
+import { registerComponent } from '../../core/component.js';
+import pkg from '../../../package.json';


Worth pointing out that importing JSON files would also not work in a no-build setup. Perhaps once JSON modules are readily available in browsers.

The new web standard will be:

import pkg from '../../../package.json' with { type: "json" };

currently supported by node if you test it with npm run test:node.
For webpack, babel-generator gives a warning

You are using import attributes, without specifying the desired output syntax. Please specify the "importAttributesKeyword" generator option, whose value can be one of: - "with" : `import { a } from "b" with { type: "json" };` - "assert" : `import { a } from "b" assert { type: "json" };` - "with-legacy" : `import { a } from "b" with type: "json";`

Not sure where you configure it.

For me webpack (or rather babel) complains that the @babel/plugin-syntax-import-attributes is missing, and after installing and adding it to the plugins of babel-loader, it works fine.

But I don't think there's any upside to pre-emptively using this syntax. In fact, I'd rather see us avoiding importing package.json altogether. Currently the entirety of package.json gets embedded into the A-Frame bundle, despite only needing the version property.

I agree with you, I wouldn't introduce the new syntax either. We should just inject the aframe version as part of the build like we do for INSPECTOR_VERSION really. Importing package.json was only to get the aframe version when importing from node AFAICT. I don't see a real use of importing aframe in node where there is no custom elements api.

It was used for logging the THREE and WebVR polyfill versions in the past as well, but the former now uses THREE.REVISION and the latter has since been removed. Injecting the version is a lot cleaner.

mrxz · 2025-01-17T17:19:33Z

src/core/readyState.js

+
+/** Resets the canInitializeElements flag, used for testing */
+export function reset () {
+  canInitializeElements = false;
+}


Anything imported through import is not mutable. The readyState.test.js used to override this value to simulate various scenarios. As a workaround this method is introduced.

An alternative would be to create a holder and mutate its contents, e.g.:

export var canInitializeElements = { value: false }

But I'd rather add this method to accommodate tests than to change all consumers because of the tests.

mrxz · 2025-01-17T17:20:59Z

src/core/scene/a-scene.js

+import { ANode } from '../a-node.js';
+import { initPostMessageAPI } from './postMessage.js';
+
+import '../../utils/ios-orientationchange-blank-bug.js';


Conditional logic is moved into this class. So while it's always "loaded", it's only applied for iOS. This is simply due to the fact that require() is synchronous, but import() isn't. Which would then either require a top-level await, or an awkwardly placed .then()

mrxz · 2025-01-17T17:23:05Z

src/index.js


 // CSS.
 if (utils.device.isBrowserEnvironment) {
+  window.logs = debug;


This used to be part of debug.js, but would be executed only when isBrowserEnvironment which is defined in device.js, which in turn depends on debug.js. This circular dependency caused issues.

mrxz · 2025-01-17T17:27:42Z

tests/__init.test.js

+var loadPromise = Promise.allSettled([
+  import('index.js'),
+  import('core/scene/a-scene.js')
+]).then(function (results) {
+  AFRAME = results[0].value.default;
+  AScene = results[1].value.AScene;
+
+  // Make sure WebGL context is not created since CI runs headless.
+  // Stubs below failed once in a while due to asynchronous test setup / teardown.
+  AScene.prototype.setupRenderer = function () {};
+});


Asynchronous loading of AFRAME and a-scene, as otherwise the above WebXR stubbing would happen too late.

mrxz · 2025-01-17T17:28:36Z

tests/components/scene/stats.test.js

    el.parentNode.removeChild(el);

-    utils.getUrlParameter = originalGetUrlParameter;
+    window.history.replaceState(null, '', originalLocationUrl);


Instead of trying to overwrite the getUrlParameter util (which is now read-only) the URL is actually rewritten using the history API.

mrxz · 2025-01-17T17:29:10Z

tests/components/scene/xr-mode-ui.test.js

+import { entityFactory } from '../../helpers.js';

-var UI_CLASSES = ['.a-orientation-modal', '.a-enter-vr-button'];
+var UI_CLASSES = ['.a-orientation-modal', '.a-enter-vr'];


The a-hidden class is applied to the wrapper around the button. The test was incorrect before.

mrxz · 2025-01-17T17:30:24Z

tests/core/controls.test.js

    canvas = sceneEl.canvas;
    sceneEl.isMobile = true;
-    canvas.clientWidth = 1000;
+    assert.isAtLeast(canvas.clientWidth, 1000);


Again an instance where one can't set values that are read-only. From my testing the line of code wasn't actually needed, but I've added an assertion to at least make sure the desired state is there.

On my Ubuntu machine, the window opens with width 886
AssertionError: expected 886 to be at least 1000
I changed it to 800 in #5641

mrxz · 2025-01-17T17:31:35Z

webpack.config.js

+    library: {
+      name: 'AFRAME',
+      type: 'var',
+      export: 'default'
+    },


This instructs WebPack to basically ensure that when loading the UMD bundle, the default export will be available under the name AFRAME. This matches the current behaviour of the bundle AFAICT.

Yes but we should still set window.AFRAME = AFRAME; ourself for the module build used with importmap, see my patch in the last comment.

vincentfretin

Wow that's an insane amount of work you did here, thank you!

vincentfretin · 2025-01-18T11:33:20Z

src/components/meta-touch-controls.js

-
-registerComponent('oculus-touch-controls', componentConfig);
-module.exports.Component = registerComponent('meta-touch-controls', componentConfig);
+});


@dmarcos kept the "oculus-touch-controls" name for backward compatibility, depending of when we merge this before or after 1.7.0, you may need to put it back.

Overlooked this while resolving merge conflicts during the rebase as my local branch was from before the rename. Will restore it so that the component is registered under both names.

Though, thinking about it, I'm not convinced this is a good way to achieve backwards compatibility. Something trivial like the following will already start behaving differently:

<a-entity id="leftHand" laser-controls="hand: left" oculus-touch-controls="buttonHighlightColor: red" raycaster="objects: [mixin='box']"></a-entity>

The laser-controls will add the new meta-touch-controls while the oculus-touch-controls attribute will cause a second instance to be added. Pulling the trigger will result in two events being emitted...

True. I think we should just remove it.

@dmarcos What is your thoughts on this? IMHO we should either remove it, or implement proper backwards compatibility.

I would maintain backwards compatibility. laser-controls and oculus-touch-controls are not meant to be used together. We can perhaps add a clause somewhere to make oculus-touch-controls a no-op if meta-touch-controls defined and viceversa. That way they can't be added together.

<a-entity id="leftHand" laser-controls="hand: left" oculus-touch-controls="buttonHighlightColor: red"></a-entity>

If they aren't meant to be used together, we shouldn't use them together in the docs here 😅

Either way, a quick search on GitHub reveals that they are being used together quite a lot. Often times it's just superfluous, but even those usages will "break" when upgrading to 1.7.0. Not to mention users directly referencing el.components['oculus-touch-controls'] or querying on [oculus-touch-controls].

Since this isn't really in the scope of this PR, I'll restore the double registration for now.

vincentfretin · 2025-01-18T11:50:53Z

src/core/component.js

-if (window.debug) {
-  var registrationOrderWarnings = module.exports.registrationOrderWarnings = {};
-}
+export var registrationOrderWarnings = {};


Double checking this change, okay I see it's imported in tests, and it's used below in registerComponent

if (window.debug) { registrationOrderWarnings[name] = true; }

it's fine 👍

src/core/system.js

src/lib/three.js

src/utils/src-loader.js

vincentfretin · 2025-01-18T12:37:43Z

tests/utils/isIOSOlderThan10.test.js

@@ -1,5 +1,5 @@
 /* global assert, suite, test */
-var isIOSOlderThan10 = require('utils/isIOSOlderThan10');
+var isIOSOlderThan10 = require('utils/isIOSOlderThan10.js');


You missed this one? src/utils/isIOSOlderThan10.js is not converted either.

Actually it can be removed. It's only referenced in this test and never exposed in the bundle. This check seems to originally be used to workaround a bug in the webvr-polyfill on Safari.

Talking about webvr-polyfill we removed. I think we should also remove the cardboardModeEnabled flag in xr-mode-ui component and documentation.

src/components/scene/stats.js

vincentfretin · 2025-01-18T12:57:51Z

src/utils/index.js

  warn('`utils.isOculusGo` has been deprecated, use `utils.device.isMobileVR`');
-};
-module.exports.isMobile = function () {
+}


So before isGearVR and isOculusGo were available on AFRAME.utils and AFRAME.utils.device I see.
Those functions are deprecated since a long time now.
Shouldn't we just remove those two functions now @dmarcos? We also removed gearvr-controls from the repo.

yeah. we can remove those functions

done in #5635

vincentfretin · 2025-01-18T13:25:39Z

src/lib/three.js

 // TODO: Eventually include these only if they are needed by a component.
-global.THREE = THREE;
-require('../../vendor/DeviceOrientationControls');
+THREE.DRACOLoader = DRACOLoader;


Before we had

var THREE = { ...SUPER_THREE };

that was needed for node that you can test with npm run test:node
you get TypeError: Cannot add property DRACOLoader, object is not extensible
without it.

We also have the error with webpack output.libraryTarget: 'module' so we need it back.
Also we should use window instead of global for a future no build context. global works only via webpack or node.

var THREE = window.THREE = {...SUPER_THREE};

Now I recall having it that way before, but changing it while testing tree-shaking, as {...SUPER_THREE} prevents most tree-shaking. But indeed, it should be {...SUPER_THREE} for the time being.

Ultimately I think it would be better to have the A-Frame code import from three and its addons directly where applicable. Then construct this "SUPER_THREE + some addons" only for the final exports and populating the global scope. But that is beyond the scope of this PR.

Yes I totally agree. For example importing LightProbeGenerator should really be done in src/components/light.js. We can do that in a separate PR.

vincentfretin · 2025-01-18T16:07:37Z

Using importmap works!

examples/boilerplate/hello-world/importmap.html

<!DOCTYPE html>                                                                    
<html>                                                                             
  <head>                                                                           
    <meta charset="utf-8">                                                         
    <title>Hello, World! • A-Frame</title>                                         
    <meta name="description" content="Hello, World! • A-Frame">                    
    <script type="importmap">                                                      
      {                                                                            
        "imports": {                                                               
          "aframe": "../../dist/aframe-master.module.min.js",                   
          "three": "https://cdn.jsdelivr.net/npm/super-three@0.172.0/build/three.module.js",
          "three/addons/": "https://cdn.jsdelivr.net/npm/super-three@0.172.0/examples/jsm/"
        }                                                                          
      }                                                                            
    </script>                                                                      
    <script type="module">                                                         
      import AFRAME from "aframe";                                                 
//      import * as THREE from "three";                                            
      import { ParametricGeometry } from "three/addons/geometries/ParametricGeometry.js";
    </script>                                                                      
  </head>                                                                          
  <body>                                                                           
    <a-scene background="color: #ECECEC">                                          
      <a-box position="-1 0.5 -3" rotation="0 45 0" color="#4CC3D9" shadow></a-box>
      <a-sphere position="0 1.25 -5" radius="1.25" color="#EF2D5E" shadow></a-sphere>
      <a-cylinder position="1 0.75 -3" radius="0.5" height="1.5" color="#FFC65D" shadow></a-cylinder>
      <a-plane position="0 0 -4" rotation="-90 0 0" width="4" height="4" color="#7BC8A4" shadow></a-plane>
    </a-scene>                                                                     
  </body>                                                                          
</html>

diff --git a/src/index.js b/src/index.js
index 07b97618..6ba0c75b 100644
--- a/src/index.js
+++ b/src/index.js
@@ -68,7 +68,7 @@ if (!window.AFRAME_ASYNC) {
   readyState.waitForDocumentReadyState();
 }
 
-export default {
+var AFRAME = {
   AComponent: Component,
   AEntity: AEntity,
   ANode: ANode,
@@ -95,3 +95,5 @@ export default {
   utils: utils,
   version: pkg.version
 };
+window.AFRAME = AFRAME;
+export default AFRAME;
diff --git a/src/lib/three.js b/src/lib/three.js
index a8e34a06..7fb4d65b 100644
--- a/src/lib/three.js
+++ b/src/lib/three.js
@@ -9,7 +9,7 @@ import * as BufferGeometryUtils from 'three/examples/jsm/utils/BufferGeometryUti
 import { LightProbeGenerator } from 'three/examples/jsm/lights/LightProbeGenerator.js';
 import { DeviceOrientationControls } from '../../vendor/DeviceOrientationControls.js'; // THREE.DeviceOrientationControls
 
-var THREE = global.THREE = SUPER_THREE;
+var THREE = window.THREE = {...SUPER_THREE};
 
 // TODO: Eventually include these only if they are needed by a component.
 THREE.DRACOLoader = DRACOLoader;

webpack.prod-module.config.js (remove libraryName, set libraryTarget 'module' with experiments.outputModule: true and externals three):

var path = require('path');
var webpack = require('webpack');
var TerserPlugin = require('terser-webpack-plugin');

module.exports = {
  entry: './src/index.js',
  output: {
    libraryTarget: 'module',
    path: path.resolve(__dirname, 'dist'),
    publicPath: '/dist/',
    filename: 'aframe-master.module.min.js'
  },
  experiments: {
    outputModule: true
  },
  externalsType: 'module',
  externals: {
    three: 'three'
  },
  devtool: 'source-map',
  mode: 'production',
  devServer: {
    port: process.env.PORT || 9000,
    hot: false,
    liveReload: true,
    static: {
      directory: 'examples'
    }
  },
  plugins: [
    new webpack.DefinePlugin({
      INSPECTOR_VERSION: JSON.stringify(
        process.env.INSPECTOR_VERSION
      )
    }),
    new webpack.ProvidePlugin({
      Buffer: ['buffer', 'Buffer']
    })
  ],
  module: {
    rules: [
      {
        test: /\.js$/,
        use: {
          loader: 'babel-loader'
        }
      },
      {
        test: /\.css$/,
        use: ['style-loader', 'css-loader']
      }
    ]
  },
  optimization: {
    minimize: true,
    minimizer: [
      new TerserPlugin({
        terserOptions: {
          compress: {
            passes: 2
          },
          format: {
            comments: false
          }
        },
        extractComments: false
      })
    ]
  }
};

This produces a ES module dist/aframe-master.module.min.js without threejs. The first line of the bundle is

import * as __WEBPACK_EXTERNAL_MODULE_three__ from "three";

npx webpack --config webpack.prod-module.config.js
cp -rf dist/ examples/
npm start

http://localhost:8080/boilerplate/hello-world/importmap.html

vincentfretin · 2025-01-18T16:26:33Z

For the npm run test:node I think we should remove that node support. I fixed it last time because I could fix it, but that is really used nowhere, nobody complained that the test was broken when we switched to three ES modules at that time, that's just useful to import aframe in node and get the version (from the doc) :D

src/constants/index.js

vincentfretin · 2025-01-19T09:56:26Z

For the npm run test:node I think we should remove that node support. I fixed it last time because I could fix it, but that is really used nowhere, nobody complained that the test was broken when we switched to three ES modules at that time, that's just useful to import aframe in node and get the version (from the doc) :D

I tried yesterday to fix it, that meant using the with { type: "json" } for importing package.json (and we need an additional babel plugin to support that syntax for the bundles), using type: "module" in package.json, renaming webpack.config.js to webpack.config.cjs, but in the end we are still blocked with the two conditional require for the css. In node, an ES module can't use require.

vincentfretin · 2025-01-19T10:40:48Z

Just reviewed again after you force pushed, your changes are all okay. 👍 The webpack module build still working properly.
Using globalThis instead of window or global is indeed correct, I completely forgot that one existed.

dmarcos · 2025-01-21T00:30:46Z

Thanks for this. Regular script tag still works right?

dmarcos · 2025-01-21T00:31:00Z

It needs rebase as well.

mrxz · 2025-01-21T08:10:47Z

Thanks for this. Regular script tag still works right?

Yes, that's the point of this PR. Switching from CommonJS to ESM internally while still outputting an UMD bundle that works like before.

With this done, it becomes possible to create an ESM bundle and using it in an importmap, as @vincentfretin demonstrates, but that isn't part of this PR.

vincentfretin · 2025-01-21T09:47:30Z

Yes, that is good to be merged for me.
I'll create a PR that create the new ESM bundle with documentation and an example after this is merged.

dmarcos · 2025-01-22T21:35:42Z

Very nice work. @vincentfretin are you working on follow up examples / docs?

dmarcos · 2025-01-23T14:29:28Z

@vincentfretin would be nice to have docs/examples with the new ways to import / consume aframe. Do you have anything handy we can incorporate to docs / readme / examples / maybe a glitch

vincentfretin · 2025-01-23T16:33:56Z

Yes, I said in the comment just above #5632 (comment) I'll do that. ;)
I did a glitch, see my comment #5363 (comment) but I'll add it in the examples in the repo. I did today #5640 to create the module bundle. I'll create tomorrow or Saturday another PR with the example and documentation.

dmarcos · 2025-01-23T17:15:40Z

Yes, I said in the comment just above #5632 (comment) I'll do that. ;) I did a glitch, see my comment #5363 (comment) but I'll add it in the examples in the repo. I did today #5640 to create the module bundle. I'll create tomorrow or Saturday another PR with the example and documentation.

Thanks so much. Just making sure I understood and we’re in same page. Really excited about getting best of both modules and old school builds.

vincentfretin · 2025-01-27T10:57:44Z

Example and doc in #5645

mrxz commented Jan 17, 2025

View reviewed changes

mrxz force-pushed the es-import branch from bb34586 to 8e662d0 Compare January 17, 2025 17:10

mrxz commented Jan 17, 2025

View reviewed changes

mrxz mentioned this pull request Jan 17, 2025

Get rid of build step #5363

Open

vincentfretin reviewed Jan 18, 2025

View reviewed changes

src/constants/index.js Outdated Show resolved Hide resolved

mrxz force-pushed the es-import branch from 8e662d0 to 3d7cf01 Compare January 19, 2025 10:23

Use import/export instead of require()/module.exports

7ff5805

mrxz force-pushed the es-import branch from 3d7cf01 to 7ff5805 Compare January 21, 2025 07:53

Merge branch 'master' into es-import

a07ab3a

dmarcos merged commit f43aa6d into aframevr:master Jan 22, 2025
3 checks passed

vincentfretin mentioned this pull request Jan 23, 2025

Relax the clientWidth constraint in the controls tests #5641

Merged

vincentfretin mentioned this pull request Feb 14, 2025

Make A-Frame ES Modules Compliant to Enable Tree Shaking #4242

Open

Uh oh!

Use import/export instead of require()/module.exports #5632

Use import/export instead of require()/module.exports #5632

Uh oh!

Conversation

mrxz commented Jan 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentfretin Jan 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentfretin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentfretin Jan 18, 2025 •

edited

Loading

vincentfretin commented Jan 18, 2025 •

edited

Loading