Skip to content

Switch to webpack to support ESM #5116

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Sep 15, 2022
Merged

Switch to webpack to support ESM #5116

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
47180de
remove budo script
vincentfretin Sep 14, 2022
63b63d1
switch build tool to webpack to support ESM modules
vincentfretin Sep 14, 2022
0dabe9f
there is no 'npm run build' anymore
vincentfretin Sep 14, 2022
021ffc4
const nit
dmarcos Sep 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,9 @@ jobs:
with:
run: npm run test:chrome -- --single-run

# - name: Test node
# run: npm run test:node

- name: Check Lint
run: npm run lint

- name: Check Build
run: npm run build

- name: Check Build Minified
run: npm run dist

Expand Down
42 changes: 16 additions & 26 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@
"homepage": "https://aframe.io/",
"main": "dist/aframe-master.js",
"scripts": {
"browserify": "browserify src/index.js -s 'AFRAME' -p browserify-derequire",
"build": "shx mkdir -p build/ && npm run browserify -- --debug -t [ envify --INSPECTOR_VERSION dev ] -o build/aframe.js",
"dev": "npm run build && cross-env INSPECTOR_VERSION=dev node ./scripts/budo -t envify",
"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",
Copy link
Contributor Author

@vincentfretin vincentfretin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"dist": "node scripts/updateVersionLog.js && npm run dist:min && npm run dist:max",
"dist:max": "webpack --config webpack.config.js",
"dist:min": "webpack --config webpack.prod.config.js",
"docs": "markserv --dir docs --port 9001",
"preghpages": "node ./scripts/preghpages.js",
"ghpages": "ghpages -p gh-pages/",
Expand All @@ -20,7 +18,7 @@
"prepush": "node scripts/testOnlyCheck.js",
"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",
Copy link
Contributor Author

@vincentfretin vincentfretin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"test": "karma start ./tests/karma.conf.js",
"test:docs": "node scripts/docsLint.js",
"test:firefox": "npm test -- --browsers Firefox",
Expand All @@ -37,6 +35,7 @@
"vendor/**/*"
],
"dependencies": {
"buffer": "^6.0.3",
"custom-event-polyfill": "^1.0.6",
"debug": "ngokevin/debug#noTimestamp",
"deep-assign": "^2.0.0",
Expand All @@ -51,25 +50,20 @@
"webvr-polyfill": "^0.10.12"
},
"devDependencies": {
"browserify": "^17.0.0",
"browserify-css": "^0.15.0",
"browserify-derequire": "^1.1.1",
"browserify-istanbul": "^3.0.1",
"budo": "^11.8.4",
"@babel/core": "^7.17.10",
"babel-loader": "^8.2.5",
"babel-plugin-istanbul": "^6.1.1",
"chai": "^4.3.6",
"chai-shallow-deep-equal": "^1.4.0",
"chalk": "^1.1.3",
"cross-env": "^7.0.3",
"envify": "^4.1.0",
"exorcist": "^2.0.0",
"css-loader": "^6.7.1",
"ghpages": "0.0.8",
"git-rev": "^0.2.1",
"glob": "^8.0.3",
"husky": "^0.11.7",
"istanbul": "^0.4.5",
"jsdom": "^20.0.0",
"karma": "^6.4.0",
"karma-browserify": "^8.1.0",
"karma-chai-shallow-deep-equal": "0.0.4",
"karma-chrome-launcher": "^3.1.1",
"karma-coverage": "^2.2.0",
Expand All @@ -78,6 +72,7 @@
"karma-mocha": "^2.0.1",
"karma-mocha-reporter": "^2.2.5",
"karma-sinon-chai": "^2.0.2",
"karma-webpack": "^5.0.0",
"markserv": "github:sukima/markserv#feature/fix-broken-websoketio-link",
"mocha": "^10.0.0",
"replace-in-file": "^2.5.3",
Expand All @@ -87,17 +82,15 @@
"sinon": "<12.0.0",
"sinon-chai": "^3.7.0",
"snazzy": "^5.0.0",
"terser": "^5.15.0",
"style-loader": "^3.3.1",
"too-wordy": "ngokevin/too-wordy",
"webpack": "^5.73.0",
"webpack-cli": "^4.10.0",
"webpack-dev-server": "^4.11.0",
"webpack-merge": "^5.8.0",
"write-good": "^1.0.8"
},
"link": true,
"browserify": {
"transform": [
"browserify-css",
"envify"
]
},
"semistandard": {
"ignore": [
"build/**",
Expand All @@ -120,9 +113,6 @@
"web-components",
"webvr"
],
"browserify-css": {
"minify": true
},
"engines": {
"node": ">= 4.6.0",
"npm": ">= 2.15.9"
Expand Down
50 changes: 0 additions & 50 deletions scripts/budo.js
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion src/components/animation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
var anime = require('super-animejs');
var anime = require('super-animejs').default;
var components = require('../core/component').components;
var registerComponent = require('../core/component').registerComponent;
var THREE = require('../lib/three');
Expand Down
2 changes: 1 addition & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ module.exports = window.AFRAME = {
AComponent: require('./core/component').Component,
AEntity: AEntity,
ANode: ANode,
ANIME: require('super-animejs'),
ANIME: require('super-animejs').default,
Copy link
Contributor Author

@vincentfretin vincentfretin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

AScene: AScene,
components: components,
coreComponents: Object.keys(components),
Expand Down
13 changes: 1 addition & 12 deletions src/lib/three.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
var THREE = global.THREE = require('super-three');
var THREE = require('./three.module.js').default;

// Allow cross-origin images to be loaded.

Expand All @@ -18,17 +18,6 @@ if (THREE.Cache) {
THREE.Cache.enabled = true;
}

// TODO: Eventually include these only if they are needed by a component.
require('../../vendor/DeviceOrientationControls'); // THREE.DeviceOrientationControls
require('super-three/examples/js/loaders/DRACOLoader'); // THREE.DRACOLoader
require('super-three/examples/js/loaders/GLTFLoader'); // THREE.GLTFLoader
require('super-three/examples/js/loaders/KTX2Loader'); // THREE.KTX2Loader
require('super-three/examples/js/loaders/OBJLoader'); // THREE.OBJLoader
require('super-three/examples/js/loaders/MTLLoader'); // THREE.MTLLoader
require('super-three/examples/js/utils/BufferGeometryUtils'); // THREE.BufferGeometryUtils
require('super-three/examples/js/lights/LightProbeGenerator'); // THREE.LightProbeGenerator
require('super-three/examples/js/utils/WorkerPool'); // WorkerPool used by KTX2Loader

THREE.DRACOLoader.prototype.crossOrigin = 'anonymous';
THREE.GLTFLoader.prototype.crossOrigin = 'anonymous';
THREE.KTX2Loader.prototype.crossOrigin = 'anonymous';
Expand Down
23 changes: 23 additions & 0 deletions src/lib/three.module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import * as SUPER_THREE from 'super-three';
import { DRACOLoader } from 'super-three/examples/jsm/loaders/DRACOLoader';
import { GLTFLoader } from 'super-three/examples/jsm/loaders/GLTFLoader';
import { KTX2Loader } from 'super-three/examples/jsm/loaders/KTX2Loader';
import { OBJLoader } from 'super-three/examples/jsm/loaders/OBJLoader';
import { MTLLoader } from 'super-three/examples/jsm/loaders/MTLLoader';
import * as BufferGeometryUtils from 'super-three/examples/jsm/utils/BufferGeometryUtils';
import { LightProbeGenerator } from 'super-three/examples/jsm/lights/LightProbeGenerator';

var objectAssign = require('object-assign');
var THREE = window.THREE = objectAssign({}, SUPER_THREE);

// TODO: Eventually include these only if they are needed by a component.
require('../../vendor/DeviceOrientationControls'); // THREE.DeviceOrientationControls
THREE.DRACOLoader = DRACOLoader;
THREE.GLTFLoader = GLTFLoader;
THREE.KTX2Loader = KTX2Loader;
THREE.OBJLoader = OBJLoader;
THREE.MTLLoader = MTLLoader;
THREE.BufferGeometryUtils = BufferGeometryUtils;
THREE.LightProbeGenerator = LightProbeGenerator;

export default THREE;
5 changes: 3 additions & 2 deletions tests/__init.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* global AFRAME, sinon, setup, teardown */
/* global sinon, setup, teardown */

/**
* __init.test.js is run before every test case.
Expand All @@ -19,13 +19,14 @@ navigator.getVRDisplays = function () {
return Promise.resolve([mockVRDisplay]);
};

require('index');
const AFRAME = require('index');
Copy link
Contributor Author

@vincentfretin vincentfretin Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var AScene = require('core/scene/a-scene').AScene;
// Make sure WebGL context is not created since Travix CT runs headless.
// Stubs below failed once in a while due to asynchronous tesst setup / teardown.
AScene.prototype.setupRenderer = function () {};

setup(function () {
window.AFRAME = AFRAME;
Copy link
Contributor Author

@vincentfretin vincentfretin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.sinon = sinon.createSandbox();
// Stubs to not create a WebGL context since Travis CI runs headless.
this.sinon.stub(AScene.prototype, 'render');
Expand Down
39 changes: 18 additions & 21 deletions tests/karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// Karma configuration.
var path = require('path');
var glob = require('glob');
var webpackConfiguration = require("../webpack.config.js");

// Define test files.
var FILES = [
Expand All @@ -14,15 +16,20 @@ if (process.env.TEST_FILE) {
}
});
} else {
FILES.push('tests/**/*.test.js');
glob.sync('tests/**/*.test.js').forEach(function (filename) {
FILES.push(filename);
});
Copy link
Contributor Author

@vincentfretin vincentfretin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// add 'src' to be able to resolve require('utils/tracked-controls') for
// example in the tests
webpackConfiguration.resolve.modules = ['src', 'node_modules'];
// webpack will create a lot of files, use build directory instead of dist
webpackConfiguration.output.path = path.resolve(__dirname, '../build');

var karmaConf = {
basePath: '../',
browserify: {
debug: true,
paths: ['src']
},
webpack: webpackConfiguration,
browsers: ['Firefox', 'Chrome'],
customLaunchers: {
ChromeTravis: {
Expand All @@ -38,28 +45,19 @@ var karmaConf = {
'TEST_ENV'
],
files: FILES,
frameworks: ['mocha', 'sinon-chai', 'chai-shallow-deep-equal', 'browserify'],
frameworks: ['mocha', 'sinon-chai', 'chai-shallow-deep-equal', 'webpack'],
preprocessors: {
'tests/**/*.js': ['browserify', 'env']
'tests/**/*.js': ['webpack', 'env']
},
reporters: ['mocha']
};

// Configuration for code coverage reporting.
if (process.env.TEST_ENV === 'ci') {
Object.assign(karmaConf.browserify, {
transform: [
[
'browserify-istanbul', {
instrumenterConfig: {
embedSource: true
},
defaultIgnore: true,
ignore: ['**/node_modules/**', '**/tests/**', '**/vendor/**', '**/*.css']
}
]
]
});
// modify the babel-loader rule
karmaConf.webpack.module.rules[0].use.options = {
plugins: [['istanbul', { 'exclude': ['**/node_modules/**', '**/tests/**', '**/vendor/**', '**/*.css'] }]]
};
karmaConf.coverageReporter = {
dir: 'tests/coverage',
includeAllSources: true,
Expand All @@ -69,7 +67,6 @@ if (process.env.TEST_ENV === 'ci') {
]
};
karmaConf.reporters.push('coverage');
karmaConf.preprocessors['src/**/*.js'] = ['coverage'];
Copy link
Contributor Author

@vincentfretin vincentfretin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

karmaConf.browsers = ['Firefox', 'ChromeTravis'];
}

Expand Down
57 changes: 57 additions & 0 deletions webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
var path = require('path');
var webpack = require('webpack');

module.exports = {
entry: './src/index.js',
output: {
library: 'AFRAME',
libraryTarget: 'umd',
path: path.resolve(__dirname, 'dist'),
publicPath: '/dist/',
filename: 'aframe-master.js'
},
devtool: 'source-map',
mode: 'development',
devServer: {
host: 'local-ip',
port: process.env.PORT || 9000,
hot: false,
liveReload: true,
static: {
directory: 'examples'
}
Copy link
Contributor Author

@vincentfretin vincentfretin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
plugins: [
new webpack.DefinePlugin({
'process.env.INSPECTOR_VERSION': JSON.stringify(
process.env.INSPECTOR_VERSION
)
}),
Copy link
Contributor Author

@vincentfretin vincentfretin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

new webpack.ProvidePlugin({
process: 'process/browser',
Buffer: ['buffer', 'Buffer']
})
Copy link
Contributor Author

@vincentfretin vincentfretin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

],
resolve: {
alias: {
three: 'super-three'
Copy link
Contributor Author

@vincentfretin vincentfretin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@vincentfretin vincentfretin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
},
module: {
rules: [
{
test: /\.js$/,
use: {
loader: 'babel-loader'
// options: {
// presets: ['@babel/preset-env'],
// },
}
},
{
test: /\.css$/,
use: ['style-loader', 'css-loader']
Copy link
Contributor Author

@vincentfretin vincentfretin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
]
}
};
Loading