Fix ES usage in node #5522

vincentfretin · 2024-05-20T20:01:21Z

Description:

The npm run test:node command was broken since we moved to using three ES imports, because of aframe/src/three.js that is requiring an ES module aframe/src/lib/three.module.js and that was not supported in node, only working in webpack environment.
And using require('aframe/src') like described in the documentation https://aframe.io/docs/master/core/globals.html#requiring-aframe-in-a-node-js-environment was also broken.
Now that node 22 supports requiring an ES module with require("somemodule.mjs") currently behind a --experimental-require-module flag, see https://nodejs.org/en/blog/announcements/v22-release-announce#support-requireing-synchronous-esm-graphs, I made an attempt to fix it.

This also simplify for me using aframe as an ES module with import "aframe/src/index.js"; and my own super-three version in a webpack project using only the Buffer polyfill in webpack config, I had issues using the process polyfill with some other dependencies. And it was a nightmare to setup a three-mesh-bvh Worker with three imports because the Worker ends up with no three at all when using THREE external and no way to include an aframe build in the Worker.

Changes proposed:

Rename lib/three.module.js to lib/three.mjs for node module resolution to work
Move the require('../../vendor/DeviceOrientationControls'); in three.js
Remove process polyfill for the dist, only using it for karma to have process.browser and process.nextTick for the tests.
Fix isBrowserEnvironment function to properly check for process undefined, also fix condition for setting window.logs to use isBrowserEnvironment
Define the three to super-three alias at the npm level instead of webpack
Use jsdom-global to set all the window properties on global
Run the test:node command with --experimental-require-module node flag, works only on node 22

…flag

vincentfretin · 2024-05-20T20:11:10Z

package.json

@@ -40,7 +40,7 @@
    "deep-assign": "^2.0.0",
    "load-bmfont": "^1.2.3",
    "super-animejs": "^3.1.0",
-    "super-three": "0.164.0",
+    "three": "npm:super-three@0.164.0",


I just asked chatgpt, this syntax may works only with npm and yarn 1.x, and may not work on yarn 2 or pnpm. I know some of you are using pnpm so we need to verify that. I tried the write a node loader to specify an alias but didn't succeed.

It was late yesterday for me, so I didn't test with pnpm. Actually it works just fine with it, really don't trust anything that chatgpt is saying, even with gpt-4o, always verify things yourself.
Just tested in the aframe repo, rm -rf node_modules package-lock.json; pnpm install it works just fine

... dependencies: + buffer 6.0.3 + debug 4.3.4 + deep-assign 2.0.0 (3.0.0 is available) + load-bmfont 1.4.1 + super-animejs 3.1.0 + three <- super-three 0.164.0 + three-bmfont-text 3.0.0 + webvr-polyfill 0.10.12 ...

dmarcos · 2024-05-21T00:02:53Z

Thanks for the detail explanation. Do we need to wait for confirmation that this works with pnpm?

mrxz · 2024-05-21T07:41:33Z

src/index.js

@@ -80,7 +80,7 @@ require('./extras/primitives/');

 console.log('A-Frame Version: 1.5.0 (Date 2024-05-03, Commit #9fe641ce)');
 console.log('THREE Version (https://github.com/supermedium/three.js):',
-            pkg.dependencies['super-three']);
+            pkg.dependencies['three']);


This would now log the npm alias npm:super-three@0.164.0 which isn't really informative to the end user. We could consider THREE.REVISION instead, although that wouldn't include patch version AFAIK, so for example 0.164.1 would simply become 164

I agree, THREE.REVISION is probably enough here. I'll make the change.

Or I can just remove the "npm:super-three@" part with pkg.dependencies['three'].replace('npm:super-three@', '')
so that stays the same.

Actually it's better THREE.REVISION. I have potentially a more recent version of super-three with patches in my project, and returning the version specified in aframe dependency is actually confusing.

mrxz · 2024-05-21T08:09:14Z

It might be a good idea to change the code in inspector.js#L19 as well. Although the webpack.DefinePlugin provides a value for it, so it doesn't actually use/need process it is quite confusing esp. now that process isn't available outside node.

vincentfretin · 2024-05-21T11:29:59Z

Thanks @mrxz for the feedback, I'll make the two changes you suggested.

…TOR_VERSION

vincentfretin · 2024-05-21T11:52:04Z

Ok I think it's all good.

…le, three version may actually be different than the version defined in aframe dependencies

vincentfretin · 2024-05-21T12:54:47Z

Just to show you what my project looks like using aframe as an ES module.

in package.json

{
  "overrides": {
    "three": "vincentfretin/three.js#super-r164-lut",
  },
  "dependencies": {
    "aframe": "aframevr/aframe#81caf89",
    "aframe-extras": "^7.3.1",
    "three": "vincentfretin/three.js#super-r164-lut"
  }
}

with currently a patch with this PR changes using patch-package

in webpack.config.js

  ...
  entry: path.resolve(__dirname, "src/index.js"),
  ...
  plugins: [
    new webpack.ProvidePlugin({
      Buffer: ["buffer", "Buffer"],
    }),
   ...
  ],
  externals: {
    // nothing
  }

in src/index.js

import "aframe/src/index.js";                                                      
import "aframe-extras/controls/index.js";                                          
import "aframe-extras/pathfinding/index.js";                                       
import "aframe-extras/misc/sphere-collider.js";

dmarcos · 2024-05-22T01:45:31Z

Thanks. Should we explore this? #5363

vincentfretin · 2024-05-22T08:42:30Z

I'm all in to make steps towards using ESM modules everywhere, if I can strip some aframe components I'm not using, or switch the renderer with some other implementation, that's clearly great for me. For no build tools, I don't know if we can achieve that, like @mrxz said in #5363 (comment) there are still some dependencies that aren't ESM.

vincentfretin · 2024-05-22T08:56:21Z

That's really awesome to just update in my project package.json

"aframe": "aframevr/aframe#08fe993"

and be done with the update, still using my three fork.

mrxz · 2024-05-22T09:17:50Z

We can already start transitioning to import syntax in preparation of going fully ESM modules. While we still have a build-step, it can take care of transpiling the non-ESM dependencies. But ultimately we'd have to decide how to deal with them (either drop or replace them). At the time I made a hackish no-build PoC and had to drop the text component as it relies on several small (mostly unmaintained) dependencies.

In any case, replacing require and module.exports with import/export is more a chore than anything difficult.

[...] if I can strip some aframe components I'm not using, or switch the renderer with some other implementation, that's clearly great for me.

Totally agreed, but there is a fundamental problem with A-Frame components. They don't communicate their dependencies. Any setAttribute or even THREE.something basically assumes it to be (globally) present. While it is possible to create a slimmed down A-Frame and only (side-effect) importing components you want, this comes with a lot of trial-and-error to ensure you've got all (transient) dependencies imported as well.

In an ideal world the dependencies would also be imported using import statements. That way the end user would only have to import what they want/need and all dependencies would be pulled in automatically. Likewise three-shaking would "just work". I have been contemplating some approached this could be done, for example allowing setAttribute to accept a component constructor:

import * as AFRAME from 'aframe';
import { SomeComponent } from 'aframe-some-component';

export const OtherComponent = AFRAME.registerComponent('other-component', {
    init: function() {
        this.el.setAttribute(SomeComponent, { color: 'red' });
    }
});

But everything can be done in gradual steps. Having the code use ESM modules internally while still compiling to a traditional bundle would already be a big step.

dmarcos · 2024-05-22T14:24:22Z

Would like to retain the script tag syntax alongside modules. While modules are useful I still think they are a worse developer experience for casual prototyping and beginners.

dmarcos · 2024-06-04T07:09:20Z

Sorry I had to revert this. It breaks the A-Frame site generation https://github.com/aframevr/aframe-site that only works with old versions of node that don't support the npm:super-three@0.164.0 syntax.

We have to find a way to upgrade the A-Frame site generation or an alternative way for this PR that doesn't enforce a new version of node.

vincentfretin · 2024-06-04T10:08:55Z

The npm: prefix feature shouldn't be related to the node version, npm version maybe, and you can upgrade the npm version that comes from the node installation with npm install -g npm@latest.
I see there is a yarn.lock in the repository, just to confirm you use npm and not yarn right?
I don't have an issue running npm install on node 22 npm 10.7.0
I have indeed currently an issue with the npm run installdocs command

aframe 0.1.0: Installing
npm [ 'install', 'aframe@0.1.0' ]
⠙Error: npm exited with exit code null, signal SIGTERM
    at ChildProcess.<anonymous> (/home/vincentfretin/workspace/aframe-site/node_modules/multidep/index.js:116:28)
    at ChildProcess.emit (node:events:532:35)
    at ChildProcess._handle.onexit (node:internal/child_process:294:12)

and for npm start I have the same issue as aframevr/aframe-site#529

vincentfretin · 2024-06-04T10:13:07Z

Can you please create an issue of what is blocking you on node 16. You mentioned an old process manager you were using I think last time. Can you give us the command you're currently using to redeploy so we can attempt to fix it. We can probably switch to pm2 if needed.

dmarcos · 2024-06-04T10:26:13Z

The npm: prefix feature shouldn't be related to the node version, npm version maybe, and you can upgrade the npm version that comes from the node installation with npm install -g npm@latest. I see there is a yarn.lock in the repository, just to confirm you use npm and not yarn right? I don't have an issue running npm install on node 22 npm 10.7.0 I have indeed currently an issue with the npm run installdocs command
aframe 0.1.0: Installing
npm [ 'install', 'aframe@0.1.0' ]
⠙Error: npm exited with exit code null, signal SIGTERM
    at ChildProcess.<anonymous> (/home/vincentfretin/workspace/aframe-site/node_modules/multidep/index.js:116:28)
    at ChildProcess.emit (node:events:532:35)
    at ChildProcess._handle.onexit (node:internal/child_process:294:12)
and for npm start I have the same issue as aframevr/aframe-site#529

Yeah the website generator doesn't support new versions of node. Server that generates the site is on v6.17.1 that doesn't support the npm:super-three@0.164.0 syntax. I removed the prefix for now so I can generate the site with v6.17.1

vincentfretin · 2024-06-04T10:37:41Z

6.17.1 of node? wow
https://hexo.io seems maintained, so we probably can update that. The aframe site is just a static site right? So if I start to spend time to update the repo so it can build on latest node and deploy on GitHub Pages, would that be ok for you? That would be one less server to pay. There may be an algolia key to set up as well? But also that this needs update, we could also use something else for the search, there are index files for search that can be generated at build time nowadays.

dmarcos · 2024-06-04T10:49:55Z

Yes. That's the most modern version I found it works. It's a static site yes and hosted on github pages already. The server monitors for changes in the website repo and deploys the site automatically. Same as we do for the master builds (It's actually the same machine on AWS)

dmarcos · 2024-06-04T10:50:41Z

Feel free to play with it. Thanks so much. If we get search to work again would be awesome but priority is to be able to deploy the site again reliably.

dmarcos · 2024-06-04T10:51:39Z

This is where the A-Frame site is hosted https://github.com/aframevr/aframevr.github.io

dmarcos · 2024-06-04T11:23:42Z

I was able to deploy the website for the new 1.6.0 version. We can remerge this PR once we can make it work with the aframe-site code base. Sorry for having to revert.

vincentfretin · 2024-06-04T11:57:29Z

No worry about the revert, we can revert the revert later. I'll put back my patch in my repo in the meantime if I need a later master version.

For the bot on the AWS machine, is it using the same node version? We didn't seem to had an issue for the build, so I'm wondering why the aframe site build wasn't working.
So your bot is pulling at regular interval the aframe-site repo, get the aframe repo that have the docs, build the docs and blog and push the build to https://github.com/aframevr/aframevr.github.io that's it?
We could probably create a GitHub CI pipeline for this now in the aframe-site repo, no need to run it on the AWS machine, but I guess you need to trigger the build when there is a change in aframe docs, that's why you have the bot.
I'm just trying to understand the whole process here before I proceed with updating anything.

dmarcos · 2024-06-04T12:54:48Z

AWS only triggers on github commit. Same as the master build bot. The build bot doesn't depend on the hexo stuff so can run on a newer version of node. If there's a way to run on Github infra instead of AWS would be def better.

vincentfretin · 2024-06-08T16:12:07Z

Let's do baby steps, first this issue aframevr/aframe-site#532

vincentfretin · 2024-06-10T17:47:08Z

With aframevr/aframe-site#532 fixed you should be able to update the node/npm version used to build aframe-site by your bot. Even with the old hexo version, the build is successful with node 22.

vincentfretin added 8 commits May 20, 2024 15:26

rename three.module.js to three.mjs

dfc9fd9

make the imports works in node 22 with --experimental-require-module …

c683860

…flag

stop polyfilling process.browser and modify isBrowserEnvironment check

97df8c9

make node test running

72128b1

alias super-three to three in package.json so it works on node

f4506a4

window.THREE is set in index.js, no need to do it in lib/three.mjs

452956b

we still need process polyfill for the browser tests

10dc0d2

update documentation about jsdom

d599f43

vincentfretin commented May 20, 2024

View reviewed changes

mrxz reviewed May 21, 2024

View reviewed changes

vincentfretin mentioned this pull request May 21, 2024

Street geo 3DStreet/3dstreet#516

Merged

vincentfretin added 2 commits May 21, 2024 13:37

strip npm:super-three@ when showing the three version in console

b28c0d7

use a global INSPECTOR_VERSION variable instead of process.env.INSPEC…

d26adfd

…TOR_VERSION

vincentfretin added 2 commits May 21, 2024 13:59

put back a comment about mocked process in isBrowserEnvironment

b9392ec

use THREE.REVISION to log three version, when using aframe as ES modu…

c910e33

…le, three version may actually be different than the version defined in aframe dependencies

dmarcos merged commit 7ac47c0 into aframevr:master May 22, 2024
1 check passed

vincentfretin mentioned this pull request May 22, 2024

Get rid of build step #5363

Open

vincentfretin deleted the test-node branch May 22, 2024 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ES usage in node #5522

Fix ES usage in node #5522

vincentfretin commented May 20, 2024

vincentfretin May 20, 2024

vincentfretin May 21, 2024

dmarcos commented May 21, 2024

mrxz May 21, 2024

vincentfretin May 21, 2024

vincentfretin May 21, 2024

vincentfretin May 21, 2024

mrxz commented May 21, 2024

vincentfretin commented May 21, 2024

vincentfretin commented May 21, 2024

vincentfretin commented May 21, 2024

dmarcos commented May 22, 2024

vincentfretin commented May 22, 2024

vincentfretin commented May 22, 2024

mrxz commented May 22, 2024

dmarcos commented May 22, 2024

dmarcos commented Jun 4, 2024

vincentfretin commented Jun 4, 2024

vincentfretin commented Jun 4, 2024

dmarcos commented Jun 4, 2024

vincentfretin commented Jun 4, 2024

dmarcos commented Jun 4, 2024

dmarcos commented Jun 4, 2024

dmarcos commented Jun 4, 2024

dmarcos commented Jun 4, 2024 •

edited

Loading

vincentfretin commented Jun 4, 2024

dmarcos commented Jun 4, 2024

vincentfretin commented Jun 8, 2024

vincentfretin commented Jun 10, 2024

Fix ES usage in node #5522

Fix ES usage in node #5522

Conversation

vincentfretin commented May 20, 2024

vincentfretin May 20, 2024

Choose a reason for hiding this comment

vincentfretin May 21, 2024

Choose a reason for hiding this comment

dmarcos commented May 21, 2024

mrxz May 21, 2024

Choose a reason for hiding this comment

vincentfretin May 21, 2024

Choose a reason for hiding this comment

vincentfretin May 21, 2024

Choose a reason for hiding this comment

vincentfretin May 21, 2024

Choose a reason for hiding this comment

mrxz commented May 21, 2024

vincentfretin commented May 21, 2024

vincentfretin commented May 21, 2024

vincentfretin commented May 21, 2024

dmarcos commented May 22, 2024

vincentfretin commented May 22, 2024

vincentfretin commented May 22, 2024

mrxz commented May 22, 2024

dmarcos commented May 22, 2024

dmarcos commented Jun 4, 2024

vincentfretin commented Jun 4, 2024

vincentfretin commented Jun 4, 2024

dmarcos commented Jun 4, 2024

vincentfretin commented Jun 4, 2024

dmarcos commented Jun 4, 2024

dmarcos commented Jun 4, 2024

dmarcos commented Jun 4, 2024

dmarcos commented Jun 4, 2024 • edited Loading

vincentfretin commented Jun 4, 2024

dmarcos commented Jun 4, 2024

vincentfretin commented Jun 8, 2024

vincentfretin commented Jun 10, 2024

dmarcos commented Jun 4, 2024 •

edited

Loading