Switch to webpack to support ESM #5116

vincentfretin · 2022-09-14T18:30:11Z

Description:

Browserify doesn't support ESM (ES6 modules), the PR #5101 broke master after it has been merged because of three r144 that removed the examples/js/KTX2Loader.js file as I said in my comment
and three.js will remove all the umd builds from their repo by the end of the year
I don't think you want to maintain umd builds yourself in the super-three repo @dmarcos so I think it's time to move away from browserify/budo. In this PR I propose to switch to webpack.

Changes proposed:

I kept the changes to the minimum, so I kept polyfilling Buffer and process.browser and for dev server I kept the same behavior for the host, port, watching files.

For the Buffer polyfill, it's needed by the buffer-equal package, dependency of node_modules/load-bmfont/package.json
and Buffer is used in node_modules/load-bmfont/lib/is-binary.js
load-bmfont is used in src/components/text.js

Webpack is using ESM first if found, then fallback to UMD. For the super-animejs package that supports both, this means we need to replace require('super-animejs') by require('super-animejs').default. This is how imported ESM build behave when importing with require().

For lib/three.js, to not change to require('lib/three.js').default everywhere I did the following:
I created a lib/three.module.js file where I now import the jsm versions instead of js versions of all the loaders and in lib/three.js I use require('./three.module.js').default; so that's the only place where we use .default like that.

Important part, import from "three" that are included in jsm files are aliased to super-three.

Although we use babel-loader, we don't do any transpiling, I didn't add the @babel/preset-env package.

I tested:

npm run start
npm run start:https generates a certificate on Ubuntu 22.04 successfully, this closes Self Signed SSL error #5084
editing a file in src or example will reload the page
TEST_ENV=ci npm run test:chrome and open the coverage tests/coverage/report/index.html
npm run build create the non minified and minified build (1.3MB, webpack is using terser with -c -m options I guess) with sourcemaps working

vincentfretin · 2022-09-14T18:43:31Z

style-loader is the equivalent of browserify-css
webpack with mode=production does the minification with terser by default. Terser is an indirect dependency of webpack, so I removed the direct dependency
sourcemaps are in a separate file automatically.

vincentfretin · 2022-09-14T18:45:39Z

tests/karma.conf.js

-  FILES.push('tests/**/*.test.js');
+  glob.sync('tests/**/*.test.js').forEach(function (filename) {
+    FILES.push(filename);
+  });


I didn't quite understand why the previous code didn't work anymore, it gave me 16 tests instead of 355 tests. Doing the glob myself like it's done when you use TEST_FILE=atestfile npm run test:chrome (the if above) fixed it.

vincentfretin · 2022-09-14T18:48:05Z

tests/karma.conf.js

    ]
  };
  karmaConf.reporters.push('coverage');
-  karmaConf.preprocessors['src/**/*.js'] = ['coverage'];


Removing this line is on purpose, this is not an error, https://github.com/istanbuljs/babel-plugin-istanbul#karma is saying to not add the coverage preprocessor.
I compared the report coverage before and after this PR, this is working properly.

vincentfretin · 2022-09-14T18:49:38Z

webpack.config.js

+      'process.env.INSPECTOR_VERSION': JSON.stringify(
+        process.env.INSPECTOR_VERSION
+      )
+    }),


If you want to know more about DefinePlugin: https://webpack.js.org/plugins/define-plugin/

vincentfretin · 2022-09-14T18:52:53Z

webpack.config.js

+    liveReload: true,
+    static: {
+      directory: 'examples'
+    }


For you to understand, before you had a specific build for budo and replaced /dist by /build in the script. In webpack-dev-server, the build is done in memory when you are accessing /dist/aframe-master.js, all the other files are served from the examples directory.

vincentfretin · 2022-09-14T18:59:03Z

webpack.config.js

+    new webpack.ProvidePlugin({
+      process: 'process/browser',
+      Buffer: ['buffer', 'Buffer']
+    })


https://webpack.js.org/plugins/provide-plugin/

I found the solution for process.browser and Buffer here https://www.alchemy.com/blog/how-to-polyfill-node-core-modules-in-webpack-5
Note that webpack gave me this information at the beginning:

ERROR in ./node_modules/buffer-equal/index.js 1:13-37 Module not found: Error: Can't resolve 'buffer' in '/home/vincentfretin/workspace/aframe/node_modules/buffer-equal' BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default. This is no longer the case. Verify if you need this module and configure a polyfill for it. If you want to include a polyfill, you need to: - add a fallback 'resolve.fallback: { "buffer": require.resolve("buffer/") }' - install 'buffer' If you don't want to include a polyfill, you can use an empty module like this: resolve.fallback: { "buffer": false }

but using resolve.fallback didn't work because buffer-equal doesn't even use require('buffer'), it just uses Buffer directly.

vincentfretin · 2022-09-14T19:00:10Z

webpack.config.js

+  ],
+  resolve: {
+    alias: {
+      three: 'super-three'


This one is super important otherwise you get three not found when importing the jsm files.

https://webpack.js.org/configuration/resolve/

vincentfretin · 2022-09-14T19:02:08Z

webpack.prod.config.js

+    filename: 'aframe-master.min.js'
+  },
+  mode: 'production'
+});


This file exists just to change the filename and the mode to production so that we have a minified build.
https://webpack.js.org/configuration/mode/

vincentfretin · 2022-09-14T19:04:18Z

webpack.config.js

+      },
+      {
+        test: /\.css$/,
+        use: ['style-loader', 'css-loader']


style-loader will inject two style tags in the DOM corresponding to the two css files we import, similar to browserify-css

vincentfretin · 2022-09-14T19:11:39Z

tests/__init.test.js

 AScene.prototype.setupRenderer = function () {};

 setup(function () {
+  window.AFRAME = AFRAME;


This one gave me a hard time. I'm not sure how this worked with browserify with karma before, I think karma-browserify injected the AFRAME variable in each file but I'm not really sure. With webpack with karma AFRAME was an empty object in some tests, defining as a global variable in setup fixed it.

vincentfretin · 2022-09-14T19:14:02Z

package.json

-    "dist": "node scripts/updateVersionLog.js && npm run dist:min",
-    "dist:max": "npm run browserify -s -- --debug | exorcist dist/aframe-master.js.map > dist/aframe-master.js",
-    "dist:min": "npm run dist:max && terser dist/aframe-master.js -c --source-map 'content=dist/aframe-master.js.map,url=aframe-master.min.js.map' -o dist/aframe-master.min.js",
+    "dev": "cross-env INSPECTOR_VERSION=dev webpack serve",


The webpack command comes from the webpack-cli package. The serve argument is available when you have the webpack-dev-server package. webpack serve use the devServer configuration in webpack.config.js

vincentfretin · 2022-09-14T19:20:02Z

package.json

    "prerelease": "node scripts/release.js 1.2.0 1.3.0",
    "start": "npm run dev",
-    "start:https": "cross-env SSL=true npm run dev",
+    "start:https": "npm run dev -- --server-type https",


I chose to use the parameter here but we could have used an environment variable and set it in the config https://webpack.js.org/configuration/dev-server/#devserverserver

vincentfretin · 2022-09-14T19:25:02Z

src/index.js

  AEntity: AEntity,
  ANode: ANode,
-  ANIME: require('super-animejs'),
+  ANIME: require('super-animejs').default,


For you to understand webpack by default is importing the package from the field 'browser', 'module', 'main' in this order of priority by default if those fields are defined in package.json. super-animejs has a module field defined pointing to an ESM build.
https://webpack.js.org/configuration/resolve/#resolvemainfields

dmarcos · 2022-09-15T00:44:10Z

Great! Much appreciated. Thanks for making a surgical as possible.

vincentfretin · 2022-09-15T08:31:17Z

tests/__init.test.js

 };

-require('index');
+const AFRAME = require('index');


Sorry for the const here :) you can modify it to var directly on master if you want.

vincentfretin · 2022-09-15T08:33:35Z

I didn't expect you to merge it so quickly, I expected at least one question lol. I guess you didn't have any because I added a lot of comments to explain the changes. ;-)

arpu · 2022-09-15T08:35:12Z

this is fantastic!

vincentfretin · 2022-09-15T08:40:55Z

@dmarcos you will see that webpack generates dist/aframe-master.js.LICENSE.txt and dist/aframe-master.min.js.LICENSE.txt, you can add them to the repository.
Can you please create a build manually and push it, or make the bot working again? Thanks.

vincentfretin · 2022-09-15T09:00:43Z

Note that in the dist there is now usage of class, but really nowadays it's safe to use https://caniuse.com/?search=class
Also three is targeting browsers >1%, see their babel config that they use with rollup to make their build.

And super-animejs is also using rollup to convert the code to ES6 (ES6 and ES2015 is the same thing) via @rollup/plugin-buble.
Aframe is using mainly ES5, ES6 for-of and now ES6 import/export, we are sure we only use supported ES6 syntax in our build, so we don't need to bother to use @babel/preset-env ourself unless you decide to use class properties or some other ES proposal in the code base.

vincentfretin · 2022-09-15T09:50:30Z

Actually there was already usage of class in the previous build with the imported loaders, so yeah it doesn't change anything.

vincentfretin added 3 commits September 14, 2022 19:37

remove budo script

47180de

switch build tool to webpack to support ESM modules

63b63d1

there is no 'npm run build' anymore

0dabe9f

vincentfretin mentioned this pull request Sep 14, 2022

Add support for KTX2 texture compression to gltf-model #5101

Merged

vincentfretin commented Sep 14, 2022

View reviewed changes

const nit

021ffc4

dmarcos merged commit 2f38f5a into aframevr:master Sep 15, 2022

vincentfretin commented Sep 15, 2022

View reviewed changes

vincentfretin deleted the switch-to-webpack branch September 15, 2022 08:41

rektide mentioned this pull request Nov 13, 2022

Use EcmaScript modules #3732

Closed

bryik mentioned this pull request Nov 13, 2022

negx.jpg is not rendered in yokohama example on MacOS Chrome? bryik/aframe-cubemap-component#39

Closed

Agupta00 mentioned this pull request Feb 13, 2023

[aframe-1.4.1] update npm run dist build process 8thwall/8frame#42

Merged

Uh oh!

Switch to webpack to support ESM #5116

Switch to webpack to support ESM #5116

Uh oh!

Conversation

vincentfretin commented Sep 14, 2022

Uh oh!

vincentfretin commented Sep 14, 2022

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!

dmarcos commented Sep 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentfretin commented Sep 15, 2022

Uh oh!

arpu commented Sep 15, 2022

Uh oh!

vincentfretin commented Sep 15, 2022

Uh oh!

vincentfretin commented Sep 15, 2022

Uh oh!

vincentfretin commented Sep 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants