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

@@ -4,6 +4,9 @@
    "ecmaVersion": 12
  },
  "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.

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

@@ -21,41 +54,11 @@ if (!window.cordova && window.location.protocol === 'file:') {

 // 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

  });

  teardown(function () {
    var el = this.sceneEl;
    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


-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

@@ -407,7 +407,7 @@ suite('rotation controls on camera with touch drag (integration unit test)', fun

    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.

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.

mrxz commented Jan 17, 2025

View reviewed changes

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

8e662d0

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mrxz commented Jan 17, 2025

mrxz Jan 17, 2025

mrxz Jan 17, 2025

mrxz Jan 17, 2025

mrxz Jan 17, 2025

mrxz Jan 17, 2025

mrxz Jan 17, 2025

mrxz Jan 17, 2025

mrxz Jan 17, 2025

mrxz Jan 17, 2025

mrxz Jan 17, 2025

mrxz Jan 17, 2025

mrxz Jan 17, 2025


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

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

Are you sure you want to change the base?

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

Conversation

mrxz commented Jan 17, 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

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