-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Dev - Full Modularisation #7131
Conversation
Grunt: browserify with brfs transform. JSHint and Qunit for testing. Modified Readme. Added package.json.
Moved Shape.Utils to ../ShapeUtils in order to prevent a cyclic dependency problem.
Using ShapeUtils now instead of Shape.Utils.
Fixed a dependency problem in SphereGeometry by using ShapeUtils.
Using the native fs module to load the glsl files.
Moved constants, backwards compatibility code and polyfills in seperate files.
@mattdesl see comment about glslify. Not sure if known issue or what. |
@vanruesc Can you open a ticket in stackgl/glslify with more details on that problem? Thanks. 😄 |
What a huge and awesome PR! I don't really have time to go through it now, but a few questions off the top of my head. Is there any way to assure correctness? With these many changes, something is bound to break. How much does the unit tests cover? How does the concatenated three.js file before/after this PR compare? You mentioned issues with cyclic dependencies. Did you consider es6 modules (using something like rollup) instead of commonjs modules? ES6 modules handles cyclic dependencies and has a cleaner syntax amongst other advantages over commonjs. Not to mention that its an actual standard that will hit browsers soon. Personally, I also prefer rollup to browserify for various reasons such as dead code elimination, how it handles external deps etc - but that isn't very relevant in this situation. Also, how does this PR affect build customizations? Modularization in any form is a huge step forward in my oppinion. Hope my questions aren't missplaced - I don't want to derail the discussion in this PR. Also, I will try to look more at the actual changes tomorrow and hopefully offer some more constructive feedback then. |
@mattdesl I opened a ticket, hope it helps! @GGAlanSmithee I share your concerns for correctness. That's why I actually went through every single file and diffed it with the latest ones in mrdoob/dev thrice. It's possible that I still overlooked some minor differences, but I don't think that there are any mistakes left. Unfortunately, I don't know of any practical way to prove that. Now that the PR is behind again, I'll have to merge the new changes as well and probably need to diff the affected files again to make sure there are no mistakes. The unit tests seem to cover a bunch of math tests, light tests and extras/geometry tests - 4378 assertions in total. I have not looked into these tests in detail and can only say that they all succeed.
There's a small risk that more problems with cyclic dependencies may pop out. However, the fact that 100% of the examples work with the new bundle leads me to believe that this is rather unlikely. I have to admit that I didn't think about build customisations, yet. There should be a way to setup Grunt tasks which only compile certain parts of the library into one bundle. Any ideas on this would be much appreciated. I wouldn't bet on ES6 modules to solve cyclic dependency problems entirely. I thought about using ES6 modules, then I saw the browser compatibility and decided to stick with browserify for now. I'd prefer ES6 modules over browserify any day of the week, but I think it's a bit too early. I'll look into rollup, thanks for the hints :) Your feedback is welcome! I want three.js to become even more awesome than it already is. |
And removed unnecessary brackets.
The es6 modules wouldn't actually be used by the browser, since the build system would transpile it to es5 while bundling to the three.js file. (Unitil browsers catch up maybe?) I have not used the build system myself, but I am refering to what @benaadams discussed in #7007 . I can't see that your changes would break it, but this PR would not be backwards compatible in that sense, since you have changed some file names, moved some code etc. |
Once converted to ES6 modules, rollup can be used on the app-side or seed-project-side, but would have no play with regards to the actual module conversion. |
@trusktr Thanks for the feedback! I'm not sure if I understand all of it right so I'd like to get things straight:
I totally agree on this one, but is it really necessary to do this first? The conversion of
How would such a global build script look like ? Do you have any examples? Or do you mean this?
I like that idea. If I got it right, then the flagship repo would just be the full, pure ES6 module that contains
I think this could be done in parallel with rewriting unit tests and code cleanups.
Roger that.
Where is the difference between Browserify + Babel(ify) and Rollup -> Browserify? Rollup does resolve all issues with cyclic dependencies in three.js from what I can tell and it uses Babel. I'll test Browserify + Babelify now to see if the crucial cyclic dependency problem is handled the same way. It should be. But I wanna make sure. |
At the moment I'm still trying to design the API (abstraction, performance, intuitiveness,...). Adding these modules in the process would increase the difficulty way too much for me. Instead of trying to transform the current code I think a better approach would be for someone to port three to a module approach in a new/different repo ( Whoever does this will have to keep it up to date though. I have seen a bunch of people doing it and then abandoning it. |
@mrdoob I'm willing to do this in my spare time (will be slow), but can I port the code base to full ES6 (class's, generators, etc)? |
Merging something like this is always going to be very hard for many reasons. However, @mrdoob, I do not think porting (I read this as making a new repo separate from three.js) is the answer for the very reason you point out - it adds maintenance overhead. The idea of modularization is to make development easier, not harder. I think the right steps to take are to first decide if the classes of THREE should be rewritten as modules (I can't tell if you want this or not) and then come up with a plan to incooperate it to the three.js repo (probably as a new branch until stable). This means a lot of changes to how things are done now, but change is not always bad, and modulare code structure brings a lot of advantages, as disscused before. I think this PR is very nice since, if nothing else, it serves as a proof of concept that it can be done. It's very aggresive in its refactoring though, which might make it harder to "accept" in lack of a better word. Maybe a better strategy is many small steps, as suggested by @trusktr. |
Yeah. Big PRs are, unfortunately, rarely merged... It's better to go step by step with small PRs. |
I was expecting this PR to not get merged (see initial post bold sentence) so it's all good. @GGAlanSmithee is right; it really was about showing that a modular architecture can be achieved with the current code base and dependency problems can be avoided. @futagoza Keeping a branch or parallel repo up to date is quite a challenge. Falling behind too much can mean death real quick. @mrdoob Would it be possible to consider the modularisation after the redesign of the API is done? Until then I'll just apply the ES6 module system for testing purposes in private and see how that goes. |
Yep! Once API gets more stable and all relevant browsers catch up with ES6 we'll start moving stuff around. |
Here's my 2c: The term "module" is a bit loaded; switching to ES6 doesn't mean we are using npm modules, it just means we are using Rollup is a cool idea, but it is only useful to other developers also using rollup (which is a very small minority). For the UMD build I feel a more stable tool (browserify/webpack + Babel) would be a wiser choice, and would also allow source transforms like ES6 seems like it would be easier to adopt on ThreeJS than CommonJS or another pattern. Babel supports feature blacklisting, so the end result is code that is really similar to current ThreeJS, but with only a few differences for In terms of separate npm modules, I don't think it's realistic to split ThreeJS core into many modules, at least not for a long time. More likely candidates for modules: things outside of core like EffectComposer, font shapes, triangulation, etc. |
II dont agree that rollup is only advantageous for other developers using rollup. On top of being an awesome es6 module bundler, it is very good at dead code elimination, allowing for very easy build customizations. (If one module isn't used - rollup eliminates it when bundling). In practise, if you dont export a module and no other code uses that module, it's not included in your three.js file. This is without Babel btw, Babel is only needed if there are other es6 features being used except import / export. Actually. In order for other devs using rollup to take extra advantage of a lib using es6 modules, that lib needs to have a npm package with the Now, I am in no way married to using rollup. If people are more used to, and have more confidence in, browserify then by all means lets use browserify. I think the most important thing to get started with this is to have a sound strategy that includes incremental refactoring, whatever tech we chose to use. *EDIT I agree that seperate npm packages should not be considered until after three.js is modularised and delivered as a singel npm package. Lets keep things simple. |
@GGAlanSmithee Any browserify/webpack users doing Keep in mind; any bundler that is chosen needs to support static file inlining (eg: brfs or glslify). |
I have to correct you here! @GGAlanSmithee is right – Rollup can generate AMD, CommonJS, UMD, ES6, or a self-executing IIFE. As a proof of concept I made three-jsnext, which converts the three.js codebase to ES6. My three.js apps are about 40% smaller as a result, with no build configuration, but even if a developer doesn't use Rollup themselves, the UMD build that it generates is actually smaller than the official build. ES6 modules === magic! |
Also, my hope with Rollup is that other module bundlers will eventually adopt similar ES6 module tree-shaking approaches (I care far more about that than people actually using Rollup itself). But the only way we'll get there is if major libraries like Three offer ES6 alongside their UMD builds, otherwise there's no incentive ;-) |
@mattdesl gotya! I still think that you are wrong though ;) The thing that three.js need to do for rollup users to take advantage of it is to simply use es6 modules (with or without rollup) and have a I was talking about the actual bundling of the three.js file. Using rollup, if you remove an export from three.js and that module isnt used internally by another module, that code is dead and so eliminated by rollup when buidling three.js, similar to current build customizations. Without the obvious hazards of removing files from a concatenated build. |
@Rich-Harris I had no idea you had actually converted three to es6 already. That's a great reference going forward with making three modularised. |
This is pretty cool and maybe a strong enough reason to switch to ES6... 😱 How does ES6 lead to a 40% smaller UMD file? What is being removed from the bundle? |
Ok, I misread your earlier statement. The 40% benefit is only useful to other developers relying on rollup, which enforces the point I was making earlier: Rollup is, currently, mainly only useful to other Rollup users. It's a great project, don't get me wrong... hopefully one day browserify/webpack will support the same dead code elimination. |
I think the 40% reduction must refer to exporting three with A standard build should be about the same using rollup or browserify. EDIT got ninjad ;) I would say it is more useful to users using rollup, but still useful to everyone else wanting customized builds. Still the most important thing would be to use es6 modules no mather what bundling tech is used imo. |
@GGAlanSmithee is right, it's only that much smaller because my app isn't using parts of Three, and I'm using Rollup. But the UMD build that's generated from the ES6 src files (i.e. it includes everything) is still a bit smaller, because in the office build everything hands off the To be fair, it comes out about the same when you gzip it, but the point is that with ES6 you don't add the overhead that you'd incur with CommonJS bundlers, so a Rollup bundle is guaranteed to be smaller than a Browserify bundle. |
Unless the browserify source is also using ES6, which seems like a good choice going forward. 😄 |
@mattdesl In my opinion, any viable build tool should be considered, for the sake of argument, what reasons do you think there are to use browserify over rollup? That it's more proven? |
Unfortunately not – unless I'm mistaken, Browserify can't deal with ES6 natively, it has to convert everything to CommonJS (e.g. with babelify). You then have the overhead of simulating the CommonJS environment (the 'prelude' at the top of a Browserify bundle) plus the overhead of each module having its own function, plus the module ID references, plus the property names that are unnecessary with ES6, etc, etc. On top of that, circular references are a giant headache with CommonJS! And Three has lots of circular references 😀 |
Ok, I took a closer look at rollup and it is indeed smaller than browserify's
Lots of reasons: Keep in mind, We could probably continue discussing bundlers for days. In the end, it doesn't matter whether webpack, browserify or rollup is chosen. And it's probably a bit premature to decide that since @mrdoob is not really looking to adopt ES6/modules immediately. 🍻 |
🎉 🎈 🍰
So true. The important thing is picking the right format for authoring – as long as libraries adopt ES6 instead of clinging to legacy formats, the tooling around it can come and go with the wind. My goal with Rollup was mostly to demonstrate some tangible benefits to ES6 modules to help nudge people along :) |
Wow, now that's great! Then there's nothing to worry about. The generated sources look really sweet.
Me too. That's something to look forward to. I feel like the bundlers will move to ES6 modules eventually for all those free advantages it brings. I can't see Browserify and co. giving up their positions as the go-to bundlers. Browserify, for example, was inspired by node's require system in the first place so I guess it will just follow as soon as node shifts to ES6. At least this appears to me as the only logical course of action. Closing with a summary:
Thanks everyone! |
Great summary! Just one very minor clarification – Rollup does have transforms which could be used for .glsl files a la brfs, albeit not currently usable via the CLI. I should update three-jsnext to do it that way rather than the premature src -> src conversion. |
Might be relevant to: #4776, #6241, #3041, #7068 and #4686
Modular Architecture
Hi,
welcome to a funny pull request with way to many changes at once.
In this case, however, it wasn't really possible to do it little by litte. I manually converted the whole codebase under src/ to a modular architecture and prepared a new build setup with Grunt.
Some prefer Gulp over Grunt. These tools can coexist so somebody could just add a Gulp configuration later.
Build process
The build process, as it is now, uses Grunt and Browserify with the BRFS transform to include all shader files. Glslify did not work, because it broke the shader chunks by adding "#define GLSLIFY 1" to every single shader, making the concatination impossible.
Additionally, JSHint is now integrated in the build process which helped with removing unnecessary semicolons, unused variables and so on. The unit tests are executed by the Qunit plugin and Uglify generates the minified version.
Changes made
I noticed that there was a mix of single and double quotes and changed everything to double quotes. Furthermore, I added curly brackets to all if-statements and moved variable declarations out of for-loops and if-blocks to prevent redeclarations and other potentially bad things.
Difficulties
When I first tried to run the examples with the new bundle, I encountered a problem with cyclic dependencies. Cyclic dependencies are, as already stated in #6241, not a serious problem. The real issue arises when objects are created from classes that aren't declared yet or when a dependency short-circuits an inheritance chain. Such problems occured in Box3, Mesh, SphereGeometry, Path, FontUtils, Shape and some other classes. The issues have been resolved for now, but might need more attention. Shape inherits from Path and Path creates Shapes. The import of the Shape class in Path had to be moved inside a function. That's really bad. Luckily, it only occured in Path.
I named the package "@mrdoob/three.js" and modified the readme file to provide basic instructions on how to use three.js as a module.
If you can't or don't want to adopt these changes as they are right now, then that's absolutely fine.
BTW: Since an npm package can only export one library, the editor and the examples are kindof lost in space. My suggestion is to move the editor into a seperate project some day with three.js as its main dependency. The same could and maybe should be done with each single example. I know that this has already been asked before (#4686). Just wanted to bring it up again in the light of this modular architecture.