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

Modules #9310

Merged
merged 5 commits into from
Jul 22, 2016
Merged

Modules #9310

merged 5 commits into from
Jul 22, 2016

Conversation

Rich-Harris
Copy link
Contributor

Bear with me, this PR is a bit of an epic...

Following the discussion on #4776, this PR converts the Three.js codebase to ES2015 modules, and updates the build process to use Rollup (full disclosure – I'm the author of Rollup).

The resulting build/three.js file is functionally identical to the existing one – the examples continue to work* without modification – albeit slightly smaller when minified, because the various modules can now refer to e.g. Mesh instead of THREE.Mesh, meaning everything gets mangled properly by UglifyJS.

Apart from the many advantages discussed in #4776 (easier development, better linting, simpler build process, etc), a great thing about using ES modules is that people will be able to do this sort of thing in their apps...

import * as THREE from 'three';

const mesh = new THREE.Mesh();
// ...

...and rather than including the entire library in the resulting bundle, ES-module aware tools that can do tree-shaking will be able to discard unused parts of the library.

Changes

The nice thing about using modules is that you no longer have to specify the order in which files should be included. The frustrating thing about modules is that you lose control over the order in which files are included.

Specifically, when you have cyclical dependencies – which Three.js has a few of – you're at the mercy of the topological sorting algorithm (per the ECMAScript spec). If you have a file like KeyframeTrack which depends on the various subclasses in animation/tracks, each of which depend on KeyframeTrack, you end up with a situation in which the prototypes of the subclasses don't inherit from the parent because the execution order is all wrong. The solution I've used here is to put the KeyframeTrack prototype and constructor function in separate files, which allows for a stable sort. A similar thing was necessary with Shape and Path.

I also moved all the constants like THREE.CullFaceNone into a separate constants.js module so that other files in the codebase can refer to them without introducing a cyclical dependency.

Similarly, the various cases of object instanceof THREE.Mesh and so on have been replaced with object && object.isMesh (and the relevant constructors have been augmented such that this.isMesh is true for all instances of Mesh. This also eliminates some nasty cyclical dependencies.

Finally, THREE.AudioContext is a bit tricky because it's a getter: with modules, THREE is just a namespace rather than an object, and while an object can have getters a namespace can't. So internally, AudioListener etc do this.context = getAudioContext() rather than this.context = AudioContext. For the UMD build, the getter is added to the export, so as far as consumers of the library are concerned THREE.AudioContext will continue to behave exactly as before.

Next steps

If you merge this (and I hope you do, but will understand if it's a bit of a leap and needs some work first!), it will pave the way for further optimisations. For example, there are lots of places where IIFEs are used to avoid polluting the global namespace – with modules, that's not necessary (everything is local), so it's possible to get rid of those. I didn't do any of that stuff with this PR because most of the changes were generated by a script (in https://github.com/rollup/three-jsnext).

Eventually this should make it easier to move certain parts out of the core library and into separate plugin repos, if that's your intention.

Let me know what you think, and thanks for reading this far!


*actually that's not quite true – the mirror / nodes example is giving me a 'Shader couldn't compile' error... I wasn't quite able to figure out what's going on, but maybe it's obvious to someone else?

@drcmda
Copy link
Contributor

drcmda commented Jul 11, 2016

@Rich-Harris @mrdoob

I tested both the legacy monolith via script-include (/build/three.js) and modules under /src/ in Webpack. Both worked flawless. The only addition one has to make in order to get it to work in Webpack is:

npm install raw-loader --save-dev

And in webpack.config.js under the loaders section:

{ test: /\.glsl$/, loader: 'raw-loader' }

Rollup seems to be a perfect fit for building this. Compared to the old build-system it is crazy how fast and lean it is. Compiling the lib took one or two seconds.

Can we please move on with this? Seeing as it doesn't seem to break examples and is already giving the source code a more logical touch, this could be the foundation of THREE-next. I feel like this is exactly what people have been asking for and it's pretty much served on a silver platter here.

@GGAlanSmithee
Copy link
Contributor

Greak work @Rich-Harris. IMO, this would be a huge step forward as far as maintainability and extensability is concerned.

To build confidence in this PR, it would be nice if the examples in the CI for this branch worked, but for some reason it seems like the three.js file is bundled but not transpiled (still contains import / export statements). Not sure why though, since the bundle looks ok in the repo.

@Rich-Harris
Copy link
Contributor Author

@GGAlanSmithee thanks – it looks like the CI server is using the old build script and concating all the src files. Is there a way I can change that in the PR so that it uses npm run build?

@bhouston
Copy link
Contributor

bhouston commented Jul 11, 2016

BTW this conversion has been done before and not previously accepted: #7131 Might be worth to compare/contrast with that PR and also look at the discussion that followed. PRs with long discussions tend not to get merged, thus try to be focused and clear.

Also this PR will mess up all other outstanding PRs. This types of breaking PRs generally should be done surgically -- planned, approved, executed and merged, the last two steps being done quickly together. Here is an example of a breaking PR that I did recently - #8223

@Rich-Harris
Copy link
Contributor Author

@bhouston Thanks. This is different to #7131 though – it introduces the fewest possible changes to the source code, doesn't add any extraneous stuff (Grunt, linting, package.json changes etc), is future-proof (no legacy CommonJS stuff) and decreases the bundle size rather than increasing it. If you review the thread it was actually closed by the author in favour of the approach presented here.

Also this PR will mess up all other outstanding PRs. This types of breaking PRs generally should be done surgically

It's not a breaking PR :-) As far as consumers of the library are concerned nothing changes.

The bulk of the work is done by a script, and the manual parts could also be scripted if the conversion needed to be rerun. That's deliberate, so that it's possible to wait for a more opportune time to do the conversion (i.e. after a few of those other PRs have been merged) if that's what the maintainers want. Honestly though, there will never be a good time to convert an entire codebase to modules, and it's very hard to do incrementally, so it may be better to just rip the bandaid off – the sooner the codebase is modularised, the sooner it'll start paying dividends.

A lot of those other PRs won't even be affected – for example https://github.com/mrdoob/three.js/pull/9307/files doesn't touch any code that this PR touches – and the rest will be very straightforward to update because the source code changes are relatively minimal. I understand that it's a short-term source of pain, but this is something that the community has been asking for for a long time, and as far as I can see that short-term pain is a) the only reason not to push forward with this, and b) outweighed by the considerable benefits.

@bhouston
Copy link
Contributor

bhouston commented Jul 11, 2016

So all examples work including those bringing in files from examples/js? Particularly do the postprocessing examples work?

Please note that mrdoob's preferred compiler is the ClosureCompiler, we have looked before to swithcing to ugly.js but haven't because ClosureCompiler produced better results.

@Rich-Harris
Copy link
Contributor Author

All the examples bar one – http://threejs.org/examples/#webgl_mirror_nodes – which works, but without the reflective splat on the floor. I got a 'Shader couldn't compile' error that I'm not sure how to debug. I checked them all, the others appear to work normally. Unfortunately I'm not yet sure how to get the CI server to use the npm run build script, so it appears broken – any advice would be appreciated.

All the postprocessing examples work identically to the ones on threejs.org/examples.

Please note that mrdoob's preferred compiler is the ClosureCompiler

No problem, can use CC instead. I just used Uglify because that's what's specified in package.json and build.js – I didn't see any references to CC?

@bhouston
Copy link
Contributor

mrdoob prefers to use the build.sh/build.bat script I believe - or at least did: https://github.com/mrdoob/three.js/blob/dev/utils/build/build.py

here is an unmerged PR that uses closure compiler in the buildjs file, very easy to use: #8366

@drcmda
Copy link
Contributor

drcmda commented Jul 11, 2016

@Rich-Harris There's a blob of code defined twice in lights_pars.glsl and ambient_pars.glsl. It ends up doubled in the shader blob because PhongNode.js includes both.

@drcmda
Copy link
Contributor

drcmda commented Jul 11, 2016

It looks to me like an error on THREEs side.

This seems to fix it:

  1. examples/js/nodes/materials/PhongNode.js, line 147, remove THREE.ShaderChunk[ "ambient_pars" ]
  2. renderers/shaders/ShaderChunk/lights_pars.glsl, replace first block with:
#include <ambient_pars>

#if NUM_DIR_LIGHTS > 0

    struct DirectionalLight {
        ...

@bhouston
Copy link
Contributor

Errors unrelated to this PR should probably be fixed in other PRs. Keeps this one clean and focused.

@drcmda
Copy link
Contributor

drcmda commented Jul 11, 2016

@bhouston It is the error that crashes webgl_mirror_nodes as mentioned by @Rich-Harris above, the only example that does not seem to work. It is probably not Rollups fault and if fixed, we have 100% coverage.

@Rich-Harris
Copy link
Contributor Author

@drcmda that does indeed fix it. Not sure why the fix wouldn't be necessary on the master branch, but I'm sure there's a reason... Happy to include the fix in this PR, just let me know what's best. Thanks.

@bhouston I've created a separate branch on my fork (based off this branch) that swaps Uglify out for CC – brings the bundle size down to 383kb. Should I merge it into this branch?

@mrdoob
Copy link
Owner

mrdoob commented Jul 12, 2016

Thanks a lot for this mammoth of a PR!! I've done a bit of review and overall looks good! 😁

However, the only thing that I'm curious about is all these new isAudio, isMesh, etc

Using this approach we can no longer do object instanceof Mesh? This surprises me a bit...

@drcmda
Copy link
Contributor

drcmda commented Jul 12, 2016

You can do object instanceof Mesh, it wouldn't affect the user or break projects. But in the modular codebase it creates lots of cyclical dependencies and adds size (imports). I believe Webpack 1 couldn't even create a lean build when everything is interlinked, even if single objects are picked out for a project.

@Rich-Harris
Copy link
Contributor Author

Exactly – for example, any project that used WebGLRenderer would automatically have to include things like Fog, FogExp2, LineSegment, LensFlare, SkinnedMesh, MultiMaterial, LineDashedMaterial, and loads of other classes that we might otherwise be able to skip (either by treeshaking, or by selectively importing the modules we actually need) in order to have a nice lightweight application. For that reason instanceof checks are sometimes considered a bit of an anti-pattern in this sort of context. See also #4776 (comment), #4776 (comment) and #6362 for previous discussions.

I don't know how important it is to have an isWhatever property for every class – for example BoxGeometry has this.isBoxGeometry = this.isGeometry = true, and while isGeometry is widely used isBoxGeometry is never used. But I thought it better to be complete and work backwards than to try and second guess these things.

@bhouston
Copy link
Contributor

Instead of the this.isBoxGeometry, we should just have this.className or this.typeName or something to that effect.

BTW if @mrdoob is in favor of merging this, we should do it soon. These types of PRs rot quickly. Going to modules (as well as the full ES2016) is needed at some point, when is the right time is up @mrdoob.

Does this change only support ES2016 modules or all of ES2016 via transpiling a la babel? With Clara,io we use babel with webpack.

@bhouston
Copy link
Contributor

I would be in favor of merging and then checking if everything works in all use cases. I would recommend doing this with at least two weeks prior to a major release. Thus maybe if @mrdoob does a release on the 15 of this month as he has sort of been doing, then we should merge this immediately after.

@Rich-Harris
Copy link
Contributor Author

Instead of the this.isBoxGeometry, we should just have this.className or this.typeName or something to that effect.

I considered that – the issue there is that an object may instantiate multiple classes. For example you might check object.isCamera or object.isObject3D depending on what you're doing – at the moment both can be true, whereas you can't have both object.className === 'Camera' and object.className === 'Object3D' for the same object.

Does this change only support ES2016 modules or all of ES2016 via transpiling a la babel? With Clara,io we use babel with webpack.

It's only modules for now – I figured other ES2015 features could come later. Very easy to integrate into the build process – you'd just add rollup-plugin-babel or rollup-plugin-buble to the plugins option in rollup.config.js (Bublé is an alternative to Babel that may be more appropriate depending on which ES2015 features you'd want to use – it's comparatively tiny, many times faster and generates leaner code, but skips features like generators that are awkward to transpile.)

@mrdoob
Copy link
Owner

mrdoob commented Jul 12, 2016

We already have .type = 'BoxGeometry' and .type = 'Geometry'. But we need to check inheritance sometimes... Would that need class stuff?

@bmeck
Copy link

bmeck commented Jul 12, 2016

Just poking in since this is a largely used module that might be affected if browser bundlers comply to Node's interop spec.

Can we avoid default exports? they lead to a ton of require('x').default, which is a big WTF.

Node core's interop spec is going to change this week after the CTC meeting. Property hoisting for CJS modules cannot be implemented like it is in babel at the VM level. It should largely not affect this PR if the consumer codebase is only ES modules, but it should be noted:

  • When this is compiled down to CJS then re-consumed by ES that there will need to be a special flag for compiled ES modules even though they run in CJS. This has not yet been standardized.

@Rich-Harris
Copy link
Contributor Author

@bmeck hey there!

Can we avoid default exports?

There's no default exports in sight, I made sure of that :-)

When this is compiled down to CJS then re-consumed by ES that there will need to be a special flag for compiled ES modules even though they run in CJS.

Presumably that's a concern for bundlers rather than library authors? Let me know if there's something Rollup needs to do – we're currently adding the widely used __esModule convention (demo).

@mrdoob

Would that need class stuff?

No, there's no need for class – the isBoxGeometry and isGeometry is designed to allow you to do the same things you were previously doing with instanceof BoxGeometry and instanceof Geometry without introducing those cyclical dependencies.

But if in future you adopt a transpiler (i.e. Babel or Bublé) then you could do this sort of thing:

import { Geometry } from '../../core/Geometry';
import { BoxBufferGeometry } from './BoxBufferGeometry';

export class BoxGeometry extends Geometry {
  constructor ( width, height, depth, widthSegments, heightSegments, depthSegments ) {
    super();

    // one of these is probably superfluous
    this.isBoxGeometry = true;
    this.type === 'BoxGeometry';

    this.parameters = { width, height, depth, widthSegments, heightSegments, depthSegments };

    this.fromBufferGeometry( new BoxBufferGeometry( width, height, depth, widthSegments, heightSegments, depthSegments ) );
    this.mergeVertices();
  }
}

export const CubeGeometry = BoxGeometry;

It would transpile something like this.

But that's all in the future – it's entirely optional, nothing depends on whether you choose to use class or the current approach to inheritance.

@mrdoob
Copy link
Owner

mrdoob commented Jul 12, 2016

No, there's no need for class – the isBoxGeometry and isGeometry is designed to allow you to do the same things you were previously doing with instanceof BoxGeometry and instanceof Geometry without introducing those cyclical dependencies.

Ah, I see what you did now...

How about something like this?

// in Geometry
this.types = [ 'Geometry' ];

// in BoxGeometry
this.types.push( 'BoxGeometry' );

// in WebGLRenderer
if ( geometry.types.includes( 'Geometry' ) ) {}

EDIT: I guess it's slower, but it basically merges both the is* and .type. .type is used, at least, by the editor to figure out what the geometry is. So it could now do...

var type = geometry.types[ geometry.types.length - 1 ]

@cecilemuller
Copy link
Contributor

cecilemuller commented Sep 7, 2016

If it helps, a single repo can be the source of multiple packages (examples: React and Babel), so each example could be its own package / module, and still be in the big monorepo.

@webprofusion-chrisc
Copy link
Contributor

A preferable (?) alternative to multiple packages is just have one package to npm install (e.g. three-toolbox) then import just the parts you need, that way the npm package admin is for published package and tree-shaking will take care of final app build size.

@Rich-Harris
Copy link
Contributor Author

What @webprofusion-chrisc said – though if it's easier to maintain individual packages from the point of view of authoring, testing, versioning etc then three-toolbox could be as simple as

// three-toolbox/src/index.js
export { default as OrbitControls } from 'three-orbitcontrols';
...

That way three-orbitcontrols is separately npm-installable for those who want that, and comes with its own ready-to-use UMD build.

@bhouston
Copy link
Contributor

bhouston commented Sep 7, 2016

I recommend we keep this all as a single repo and npm package for now as that has been sort of what ThreeJS has done so far. I prefer to keep it as this simple way for now. Once modularization of the examples is done, one can explore splitting it up into repos or npm repos, but even then I think it isn't necessary as one can have submodules in a single npm repo that can be included individually so the tree shaking can still be very effective.

@trippedout
Copy link

@bhouston @Rich-Harris any updates on the modularization of examples/toolbox? been trying to convert a few on my own with limited luck (VREffect/VRControls)

@webprofusion-chrisc did u open up another issue for it? haven't found anything and #9562 hasn't seemed to get anywhere

@webprofusion-chrisc
Copy link
Contributor

@trippedout #9562 seems to be the right discussion, event though it's at stalemate. The key argument here is support for old JS vs modules. I think transpiling (I use typescript but Babel would do it) could probably(?) support both approaches allowing modules to be written out as THREE.* namespaced JS. Only a theory, I'm unlikely to get time to try it out.

@trippedout
Copy link

@webprofusion-chrisc great thanks Chris.

@danrossi
Copy link

danrossi commented Oct 5, 2016

I've also started converting some examples to modules with some mods. It's not pure es6 though until the rest of it is. It could help with the event handlers to use arrow functions for instance.

https://github.com/danrossi/three-vr-orbitcontrols

aardgoose pushed a commit to aardgoose/three.js that referenced this pull request Oct 7, 2016
* convert to ES modules

* rebuild

* move shaders back into glsl files

* reinstate polyfills
@legodude17
Copy link

@mrdoob

Is there a way of automatically adding // threejs.org/license at the top of three.min.js?

Off the top of my head, you could use rollup-plugin-intro.

@mrdoob
Copy link
Owner

mrdoob commented Jan 28, 2017

Off the top of my head, you could use rollup-plugin-intro.

Unfortunately it doesn't look too nice... #10645 It would be nice if closure compiler allowed adding a header or something.

@cecilemuller
Copy link
Contributor

cecilemuller commented Jan 28, 2017

Worst case scenario, we could always have an extra step that adds the header to the output file after the closure command run.

@mrdoob
Copy link
Owner

mrdoob commented Jan 29, 2017

Worst case scenario, we could always have an extra step that adds the header to the output file after the closure command run.

Yeah, I did look at options at the time, but the options seemed over complicated and/or required more dependencies.

import shadow_frag from './ShaderLib/shadow_frag.glsl';
import shadow_vert from './ShaderLib/shadow_vert.glsl';

export var ShaderChunk = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This conserves ShaderChunk as mutable. Are there existing uses or valid use cases for adding to ShaderChunk? Otherwise, AFAICT this export could be export *;? @Rich-Harris : Would that affect tree-shaking of individual shaders?

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.