Modules plus docs

[adroitwhiz]

As promised, here are the changes from both my modules PR and the jsdocs branch. I've made these changes in a different branch from #425 just in case.

I'm not really sure which branch this should be merged into, so I went with jsdocs. This will probably be nearly impossible to manually merge into jsdocs, so you may want to replace jsdocs with this branch or just create a new branch instead.

The docs are still a tad broken--if you want me to take another pass over them before merging, I can do so, but this is a big complicated PR as is, so I think it'd be best to do that in another PR.

[adroitwhiz]

If it helps with reviewing, I could also create a separate PR that only includes the removal of Two.Utils' underscore.js functions.

[jonobr1]

This is awesome! If you don't mind, can you split this into to two PRs? This is a lot to go through! Excited to merge it all in and make this the jsdocs branch.

[jonobr1]

With this PR merged (link). Where does this leave us in regards to this PR?

[adroitwhiz]

I rebased the "modules" changes on top of #445 in a different branch, which I can create a different PR for.

It looks like the jsdocs branch doesn't yet include #445. Do you want me to submit the "modules" PR against dev, and then put the JSDoc changes in a different PR? The alternative would be for you to delete the jsdocs branch on GitHub (or rename it to old-jsdocs or similar) and then replace it with this one.

[jonobr1]

Got it, I'll make this PR the new jsdocs once I've gone over it. I think there will need to be some additional upstream merges with dev though 🤔 I made some changes to dev in the last day or so.

[adroitwhiz]

Have those changes been pushed yet? I have all the December changes merged, and the version number/readme updates shouldn't be too hard to include.

[adroitwhiz]

Also, is there any reason to keep jsdocs separate from dev now that 0.7.0 is out?

[jonobr1]

Have those changes been pushed yet? I have all the December changes merged, and the version number/readme updates shouldn't be too hard to include.

These have all been pushed to dev. I guess you're right. While not complete, I guess we can merge this into dev and just continue to update it there.

[adroitwhiz]

I think it's fine to have half-finished things in dev, as long as they don't break the build. It'll also become a lot easier to release alpha/beta builds once a deploy script (#440) is created.

[adroitwhiz]

Alternatively, I just rebased #425 so you can merge that into dev if you don't want the jsdocs changes.

[jonobr1]

I imagine this is for the new builds. When I npm install I get errors on a dependency that I think that version of rollup is requesting:

Do you get these errors too?

[adroitwhiz]

I don't. What versions of Node and NPM are you running?

[jonobr1]

I understand paradigms like the file src/utils/get-ratio.js which is accessible at Utils.getRatio, but this doesn't share that paradigm and the accessor is different from the previous version of Two.js. What's the thinking here?

[jonobr1]

original location was Two.Utils.defineProperty

[adroitwhiz]

I thought defineGetterSetter was a clearer name, since it has different behavior to Object.defineProperty.

[jonobr1]

Sorry for the flurry of small single line comments. I haven't even gone through the files you changed that don't show their diffs in the github web editor. But, I can't reiterate how awesome this is general! It's really putting Two.js into the 2020's, something my developer knowledge is too antiquated to do. So thank you and these nitpicks are more open ended questions to about long term ways to maintain the project. Looking forward to your thoughts on it and awesome work!

[jonobr1]

Couple more things I noticed:

1. When I try to build there are circular dependency warnings. Do you get this?

Circular dependency: src\utils\math.js
->src\matrix.js -> src\utils\math.js
Circular dependency: src\effects\texture.js
-> src\utils\canvas-shim.js -> src\renderers\canvas.js
-> src\group.js -> src\effects\texture.js

2. There's a new error in SVG rendering in the tests. Do you get this when running the tests?
   

3. Not specifically related to this PR, but something that I started thinking about when reviewing this PR. Three.js when moving to ES6 Module declarations moved away from defining functions as var name = function() {}; to the more omnipresent function name() {}. Do you have any opinions on this pattern?

[adroitwhiz]

1. I get these too-- they don't cause the build to fail, but it's probably good to get rid of them. I've removed one already; the other is caused by getComputedMatrix and decomposeMatrix in utils/math.js depending on Matrix. Does it make sense to move those functions into matrix.js?

2. For some reason, I accidentally removed a check in Two.makePath. The check has been re-added, and the tests all pass now. Good catch!

3. I have no opinions one way or the other. I'm not sure why the Three.js PR changed them.

[adroitwhiz]

@jonobr1 I still need clarification on points 1 and 3.

[jonobr1]

Sorry for the delay on this @adroitwhiz. To answer those specific questions:

1. Yeah, I think it makes sense to move those functions to the Matrix module.
   ...
2. Three.js moves to function name() {} declaration for debugging purposes. I think it's clearer in a stack trace what the name of the function is when it's declared like that instead of var name = function() {}. But, don't worry about that right now. We can update that later.

I'm still in the thick of my thesis writing.., so I unfortunately won't have a ton of time in the next couple of weeks to look through the rest of the changes and run more tests / debugs. But, I definitely appreciate and certainly haven't forgotten! It'll be the first thing I spend time on once I'm done.

[jonobr1]

I made some name changes and updated to the current dev branch. One thing I ran into that I was wondering I could get your help on has to do with Two.Utils now being hidden. Any chance you can expose Two.Utils the same way that v0.7.0 does? I tried to do it myself, but I keep getting errors. These are the additions / subtractions I amended to /src/two.js:

// Utils

import CanvasShim from './utils/canvas-shim.js';
import Curves from './utils/curves.js';
import dom from './utils/dom.js';
import Error from './utils/error.js';
import getRatio from './utils/get-ratio.js';
import defineGetterSetter from './utils/get-set.js';
import interpretSVG from './utils/interpret-svg.js';
import math from './utils/math.js';
import Commands from './utils/path-commands.js';
import root from './utils/root.js';
import interpretSVG from './utils/interpret-svg.js';
import xhr from './utils/xhr.js';
import _ from './utils/underscore.js';
import xhr from './utils/xhr.js';

// Core Classes

import Anchor from './anchor.js';
import Collection from './collection.js';
@ -15,6 +26,8 @@ import Shape from './shape.js';
import Text from './text.js';
import Vector from './vector.js';

// Effects

import {Gradient, Stop} from './effects/gradient.js';
import ImageSequence from './effects/image-sequence.js';
import LinearGradient from './effects/linear-gradient.js';
@ -22,6 +35,8 @@ import RadialGradient from './effects/radial-gradient.js';
import Sprite from './effects/sprite.js';
import Texture from './effects/texture.js';

// Secondary Classes

import ArcSegment from './shapes/arc-segment.js';
import Circle from './shapes/circle.js';
import Ellipse from './shapes/ellipse.js';
@ -31,16 +46,14 @@ import Rectangle from './shapes/rectangle.js';
import RoundedRectangle from './shapes/rounded-rectangle.js';
import Star from './shapes/star.js';

// Renderers

import CanvasRenderer from './renderers/canvas.js';
import SVGRenderer from './renderers/svg.js';
import WebGLRenderer from './renderers/webgl.js';

import Constants from './constants.js';

/**
 * @name Two
 * @class
@ -890,7 +903,18 @@ _.extend(Two, {
   * @name Two.Commands
   * @property {Object} - Map of possible path commands. Taken from the SVG specification.
   */
  Commands: Commands,

  Utils: _.extend({

    Error: Error,
    getRatio: getRatio,
    defineGetterSetter: defineGetterSetter,
    read: interpretSVG,
    xhr: xhr

  }, CanvasShim, math, _)

});

export default Two;

The error I get from roll up is:

UnhandledPromiseRejectionWarning: Error: 'Curve' is not exported by src\utils\curves.js

I think with Two.Utils exposed I'm happy to merge this into dev and begin just moving forward with modules. Awesome work! Thanks so much for your contributions.

[adroitwhiz]

The import thing from file syntax (with no brackets) will import the "default" export from file under the name thing. This only works if that file has an export default thing.

curves.js has no default export, but explicitly names everything it exports:

export {
  Curve,
  getComponentOnCubicBezier,
  subdivide,
  getCurveLength,
  getCurveBoundingBox,
  integrate,
  getCurveFromPoints,
  getControlPoints,
  getReflection,
  getAnchorsFromArcData
};

This means you need brackets to import from it:

import {Curve, getComponentOnCubicBezier} from './curves.js'

Or you can just import all the exports at once with:

import * as Curves from './curves.js'

This is somewhat complicated, but this guide might help a bit.

Two other things:

• It looks like, in the code you posted, you never extended Two.Utils with Curves.
• It also appears you imported utils/error.js as Error. This will override the regular JS Error class; if this isn't intentional, I'd import it as TwoError instead.

[jonobr1]

Thanks for the second pair of eyes. I'll make those changes now.

[jonobr1]

I'm sure there will be some additional growing pains, but thanks so much for this PR. Excited to be moving Two.js closer toward the present state of JavaScript development.