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

ESM and jsdoc #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ESM and jsdoc #8

wants to merge 1 commit into from

Conversation

brettz9
Copy link

@brettz9 brettz9 commented Apr 26, 2019

Builds on PR #7, so I'd appreciate a review of that first, and I can rebase this as needed; but you can see just these changes by looking at its latest commit:

  • Enhancement: ESM distribution and point to source via package.json module
  • Refactoring: ESM in source; change Bezier functions to individual exports
  • Docs: Fix jsdoc item; Add jsdoc config
  • npm: Add rollup script; add jsdoc script and scripts/server for opening docs; update deps

I havent yet refactored to ES6 (with Babel) besides the modules (as proposed in my previous PR), as wanted to get your approval first.

I only converted the Bezier functions to individual exports (the ESM equivalent of exports instead of module.exports), though I could have done so in some other cases. If you are open to it, I believe that should probably be done as well so that importers only need import those functions/classes they want, but I can revert even the Bezier functions if you want.

@brettz9 brettz9 force-pushed the esm branch 5 times, most recently from 519dfdc to d7a4d22 Compare April 28, 2019 00:59
…` `module`

- Refactoring: ESM in source; change Bezier functions to individual exports
- Docs: Fix jsdoc item; Add jsdoc config
- npm: Add `rollup` script; add it and `eslint` to test script; add `jsdoc` script and
    scripts/server for opening docs; update deps
@brettz9
Copy link
Author

brettz9 commented May 3, 2019

Thanks for accepting the last pull... FWIW, it seems the owner of kld-intersections has given a lot of feedback on my initial PR, so we can see what comes out of it as far as meeting your needs. I have rebased this PR too though, if you do care to take these changes now.

@brettz9
Copy link
Author

brettz9 commented May 12, 2019

kld-intersections has since integrated my linting and ESM changes, so if you end up utilizing their code, this PR should not be necessary (though there'd still need to be a simple Rollup routine).

As far as working on top of the UI, I still think svg-intersections' approach would be warranted, but the kld-intersections author did not want to include this, so I think there is indeed still a need for svg-intersections, whose API has the following advantages in making explicit the properties:

  1. Ensure calling code is more readable and with a lesser burden on memory of argument order (e.g., shape('circle', {cx: 50, cy: 100, r: 300})), also possibly allowing for aliases (e.g., accepting radius or r).
  2. Where arguments are optional, allows for only those arguments actually used to be passed in (no need for passing null to skip over an argument, for example).
  3. When passing back objects, object destructuring allows for getting out only the properties desired (more cleanly and with less burden on the memory than array destructuring, e.g., const {radius} = circleProperties; rather than const [, , radius] = circleProperties).

I think you could even simplify further by allowing for just:

circle({cx: 50, cy: 100, r: 300});

I made my own shim for your approach as below, but it is not complete.

function shape ({shape, props}) {
  let args;
  switch (shape) {
  default:
    throw new TypeError('Unexpected shape ' + shape);
  case 'rect': {
    const {x, y, width, height} = props;
    args = new IntersectionArgs('Rectangle', [
      new Point2D(x, y),
      new Point2D(x + width, y + height)
    ]);
    break;
  } case 'circle': {
    const {cx, cy, r} = props;
    args = new IntersectionArgs('Circle', [new Point2D(cx, cy), r]);
    break;
  } case 'polygon': {
    const {points} = props;
    args = new IntersectionArgs('Polygon', [points]);
    break;
  }
  }
  return args;
}

Also I'd hope you could continue to expose Intersections.intersect as just intersect.

In short, I'd recommend taking a look at https://github.com/thelonious/kld-intersections/blob/development/lib/AffineShapes.js and https://github.com/thelonious/kld-intersections/blob/development/lib/Shapes.js to see if you think it is enough to just continue to follow the IntersectionArgs approach as I did above, or whether you want to ask of more hooks from the kld-intersections author.

Btw, he has already provided https://github.com/thelonious/kld-intersections/blob/development/lib/SvgShapes.js , so there is no need for a new shim for supplying SVG elements, unless you wanted an SVG-element version of intersect, e.g.:

svgElementIntersect(
    svgElementShape1,
    svgElementShape2
);
Intersection.intersect(
    SvgShapes.element(svgElementShape1),
    SvgShapes.element(svgElementShape2)
);

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.

1 participant