Skip to content
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.

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

Examples: Revive JSM -> JS Rollup script #20527

Closed
wants to merge 6 commits into from

Conversation

gkjohnson
Copy link
Collaborator

Reviving the rollup script from #15526 / #15543 with some changes to convert the examples/jsm files to examples/js and address #20455 so we can start editing the module examples directly.

This addresses some of the issues I brought up in #20455 (comment) by only building files that already exist in the examples/js directory and making those files interdependent if needed. If a shared module is not already present in the js directories then it is inlined in the dependent file. This means in some cases there could be redundant code bundled across multiple files if a module is shared but it keeps the resulting structure clean (and backwards compatible). In order to add a file that is not already being built we just have to create an empty file in the appropriate spot and rebuild.

Using UMD also addresses the concerns brought up in #17220 by enabling the built scripts to be used as commonjs imports and therefore function in code sandbox, though this might bring more confusion and I don't believe solves a problem anywhere else.

The remaining issues that have to be worked out:

  • The files in /libs need some special handling to properly use a global variable that is not THREE -- this will probably require hand-maintaining a list of lib -> global variable mappings but fortunately that list is short.

  • The files are not as clean as the previous versions in part because of the UMD syntax (which can be removed) making hand editing them less viable. Even if we stick to just global variables rollup will wrap the files in an iife and generated variable names. With some fancy regex it might be possible to clean this up automatically. One thing to keep in mind is that we may not be able to get rid of the function wrapper because the modules may declare functions that could pollute the global namespace otherwise.

You can see an example of an output file with dependencies below. I'll await feedback / direction before putting more time into this. cc @mrdoob @Mugen87 @donmccurdy

Example output for Line2.js in IIFE format
/**
 * Generated from 'examples/jsm/lines/Line2.js'
 */

(function (exports, LineSegments2_js, LineGeometry_js, LineMaterial_js) {
	'use strict';

	var Line2 = function ( geometry, material ) {

		if ( geometry === undefined ) geometry = new LineGeometry_js.LineGeometry();
		if ( material === undefined ) material = new LineMaterial_js.LineMaterial( { color: Math.random() * 0xffffff } );

		LineSegments2_js.LineSegments2.call( this, geometry, material );

		this.type = 'Line2';

	};

	Line2.prototype = Object.assign( Object.create( LineSegments2_js.LineSegments2.prototype ), {

		constructor: Line2,

		isLine2: true

	} );

	exports.Line2 = Line2;

}(this.THREE = this.THREE || {}, THREE, THREE, THREE));
Example output for Line2.js in UMD format
/**
 * Generated from 'examples/jsm/lines/Line2.js'
 */

(function (global, factory) {
	typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('..\\LineSegments2.js'), require('..\\LineGeometry.js'), require('..\\LineMaterial.js')) :
	typeof define === 'function' && define.amd ? define(['exports', '..\\LineSegments2.js', '..\\LineGeometry.js', '..\\LineMaterial.js'], factory) :
	(global = typeof globalThis !== 'undefined' ? globalThis : global || self, factory(global.THREE = global.THREE || {}, global.THREE, global.THREE, global.THREE));
}(this, (function (exports, LineSegments2_js, LineGeometry_js, LineMaterial_js) { 'use strict';

	var Line2 = function ( geometry, material ) {

		if ( geometry === undefined ) geometry = new LineGeometry_js.LineGeometry();
		if ( material === undefined ) material = new LineMaterial_js.LineMaterial( { color: Math.random() * 0xffffff } );

		LineSegments2_js.LineSegments2.call( this, geometry, material );

		this.type = 'Line2';

	};

	Line2.prototype = Object.assign( Object.create( LineSegments2_js.LineSegments2.prototype ), {

		constructor: Line2,

		isLine2: true

	} );

	exports.Line2 = Line2;

})));

@DefinitelyMaybe
Copy link
Contributor

cool as. turns out folks were looking into this before. keep up the good work.

@donmccurdy
Copy link
Collaborator

}(this.THREE = this.THREE || {}, THREE, THREE, THREE));

This is a bit strange, should rollup be deduping these?

Using UMD also addresses the concerns brought up in #17220 by enabling the built scripts to be used as commonjs imports and therefore function in code sandbox, though this might bring more confusion and I don't believe solves a problem anywhere else.

This also improves life somewhat for people using NodeJS, or older bundlers like Browserify. But I'm also OK with the IIFE output. Not sure if Rollup would let us get the best of both with a footer like this?

if ( typeof exports === 'object' && typeof module === 'object' ) {

	module.exports = Foo;

} else {

	THREE.Foo = Foo;

}

@gkjohnson
Copy link
Collaborator Author

@donmccurdy

}(this.THREE = this.THREE || {}, THREE, THREE, THREE));

This is a bit strange, should rollup be deduping these?

It definitely could but it seems that rollup creates a new argument for every dependency no matter what. If you don't supply a global name for a dependency rollup "guesses" with a unique global name based on the dependency file name.

This also improves life somewhat for people using NodeJS, or older bundlers like Browserify. But I'm also OK with the IIFE output. Not sure if Rollup would let us get the best of both with a footer like this?

I don't think we can get away from some kind of iife function wrapper even if it did (which I don't think rollup affords this kind of control anyway). Consider a module that does the following:

const tempVec = new Vector3();

function helperFunction() {

    // ...

}

export class ExampleClass {

    // ... ExampleClass uses helperFunction

}

With just a footer helperFunction and tempVec (I think? I'm actually not sure with const) will be defined in the global scope which is not the intent when defining that function or variable in a module. Of course we could disallow such functions outside of class definitions as a rule but I would expect this is a pattern we'd want to use in the example modules and is one that is used throughout the core source files, as well.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Oct 17, 2020

No objection to the presence of an IIFE wrapper, I agree that it's probably the minimum wrapper for a generated file here. My argument is more that I prefer the functionality of UMD — really just its CJS support — while preferring the readability of IIFE. I was hoping that a suffix like the one I suggest above could just be appended to the IIFE output to get the best of both.

EDIT: I guess it's not quite so simple when considering imports, too...

@gkjohnson
Copy link
Collaborator Author

My argument is more that I prefer the functionality of UMD — really just its CJS support — while preferring the readability of IIFE. I was hoping that a suffix like the one I suggest above could just be appended to the IIFE output to get the best of both.

Oh I see -- yes I agree. Unfortunately I don't think rollup has an option to build an IIFE and Commonjs without AMD. I'll have a closer look at some of the capabilities of the rollup plugins to see if they afford some more control over something like this.

@gkjohnson
Copy link
Collaborator Author

It doesn't look like Rollup has any support for custom output wrappers right now. There have been requests and discussions related to adding support for custom wrappers (rollup/rollup#2190, rollup/rollup#1326, rollup/rollup#269) but it seems interest hasn't been strong enough to add them. Other suggestions involve using regex or something like acorn to process the bundle before output.

I toyed around with a renderChunk plugin for deduping the iife arguments and making them human readable -- the new output and plugin contents are below. Handling of the /libs files is still TBD but I think the result is more agreeable and less noisy. I think it's worth defining what an acceptable amount of "wrapping" in the final output is and what the target file structure should be so we can work towards that -- which something maybe only @mrdoob can really weigh in on:

Plugin for replacing iife arguments
renderChunk: function( code ) {

	let finalCode = code;

	// regex for parsing the iife function arguments and passed parameters list
	const argsRegex = /\(([^(]+?)\)/;
	const passedArgsRegex = /\(([^(]+?)\)([^(]+)$/;

	// get the iife arguments and parameters list
	const args = code.match( argsRegex )[1].split( /,/g ).map( a => a.trim() );
	const passedArgs = code.match( passedArgsRegex )[1].split( /,/g ).map( a => a.trim() );

	// map with argument name to passed parameter names
	const argsToPassedMap = new Map();
	const finalArgs = new Set();
	args.forEach( ( a, i ) => {

		argsToPassedMap.set( a, passedArgs[ i ] );

	} );

	// remove exports argument because we will add it first as "THREE" manually
	argsToPassedMap.delete( 'exports' );

	// convert the argument names to human readable ones and replace the original names
	// with the human readable ones through out the code
	argsToPassedMap.forEach( ( value, key ) => {

		if ( value === 'THREE' ) {

			finalCode = finalCode.replace( new RegExp( key, 'g' ), 'THREE' );
			finalArgs.add( 'THREE' );

		} else {

			// TODO: handle libraries and add them to args list

		}

	} );

	// Delete 'THREE' because we add it manually at the beginning of the arguments list
	finalArgs.delete( 'THREE' );

	const finalArgsArray = Array.from( finalArgs );
	finalArgsArray.unshift( 'THREE' );

	// replace the iife arguments and passed parameters lists with our new deduped and human
	// readable names lists
	finalCode = finalCode
		.replace( argsRegex, `( ${ finalArgsArray.join( ', ' ) } )` )
		.replace( passedArgsRegex, `( ${ finalArgsArray.join( ', ' ) } )$2` )

	return finalCode.replace( /exports/g, 'THREE' );

}
Line2 output with IIFE post processing
/**
 * Generated from 'examples/jsm/lines/Line2.js'
 */

(function ( THREE ) {
	'use strict';

	var Line2 = function ( geometry, material ) {

		if ( geometry === undefined ) geometry = new THREE.LineGeometry();
		if ( material === undefined ) material = new THREE.LineMaterial( { color: Math.random() * 0xffffff } );

		THREE.LineSegments2.call( this, geometry, material );

		this.type = 'Line2';

	};

	Line2.prototype = Object.assign( Object.create( THREE.LineSegments2.prototype ), {

		constructor: Line2,

		isLine2: true

	} );

	THREE.Line2 = Line2;

}( THREE ));

@donmccurdy donmccurdy mentioned this pull request Oct 19, 2020
@donmccurdy
Copy link
Collaborator

/cc #20529

I think it's worth defining what an acceptable amount of "wrapping" in the final output is and what the target file structure should be so we can work towards that -- which something maybe only @mrdoob can really weigh in on.

Yes, probably worth getting input from @mrdoob here before spending too much time on any particular path.

We'll also need to decide whether we actually want to generate /js versions of features that currently exist only in /jsm. For example, ZSTDDecoder, KTX2Loader, and NodeMaterial.

@Mugen87 Mugen87 mentioned this pull request Oct 29, 2020
43 tasks
@gonnavis
Copy link
Contributor

Hello @gkjohnson, how to use this Rollup script?

I want to auto convert my pr #20798 from jsm/ to js/, but when I use node utils\build\rollup-examples.config.js like node utils/modularize.js, I get this error:

(node:37364) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
C:\WWW\lib\three.js_github_gonnavis\utils\build\rollup-examples.config.js:107
export default files.map( p => createOutput( p, files ) ).filter( p => ! ! p );
^^^^^^

SyntaxError: Unexpected token 'export'

Yes, it should fail, because judging from the file name and path, this is a build config file and cannot be run directly.

So then I ran npm run build, but all I get are three.js three.min.js three.module.js, no any files under exmaples/js/.

How to use this Rollup script to auto convert jsm/ to js/?

@gkjohnson
Copy link
Collaborator Author

@gonnavis I just added a build-examples script to the package.json so you can run the conversion more easily.

@gonnavis
Copy link
Contributor

@gkjohnson Thanks! Let me try.

@gonnavis
Copy link
Contributor

@gkjohnson How about also auto-generate js version examples/***.html for testing js/ files?

What's your work flow of updating and testing exmaples? Change js/ or jsm/ first? Testing both?

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Dec 10, 2020

@gonnavis I don't plan to work on this anymore until there's a more explicit direction for the conversion. If it's suggested that this approach looks good and / or needs tweaks or testing I can put time into whatever's needed.

Regarding testing I'm confident in the results of Rollup -- it's a well maintained tool that's widely used and performs minimal transformations to the code. That's not to say testing shouldn't be done (at the least my small plugin functions need to be validated) but I think converting every html example is overkill and an unnecessary effort. I think we can just test a few of the most complex cases by hand.

@gonnavis
Copy link
Contributor

@gkjohnson OK, thanks again for your pr!

@gonnavis
Copy link
Contributor

gonnavis commented Dec 11, 2020

Success! Only the one html file is manually modified, all other js version files are auto-generated, even the new added misc/Resizer.js. @gkjohnson 👍

https://raw.githack.com/gonnavis/three.js/UnifyPcMobUnrealBloomPassJS/examples/webgl_postprocessing_unreal_bloom_js.html

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 6, 2021

Closing in favor of #21584.

@Mugen87 Mugen87 closed this Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants