-
Notifications
You must be signed in to change notification settings - Fork 55
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
ES6 modules #37
ES6 modules #37
Conversation
d44ea02
to
cb7a2a1
Compare
…` `module` (Note: not ignoring dist as can more easily flag changes or need for changes) - Refactoring: ESM in source (removing now redundant `use strict`); change Bezier functions to individual exports - npm: Add `rollup` script; add it to test script
Just an FYI that it might take a day or two before I get back to you on this. I'm thinking about applying the steps in your commits to a lesser module of mine to get a better feel of the final result. That being said, it's looking good to me. I do have one question, however. Will it be required to fully-qualify types in JSDoc?. For example:
The |
I've ported these changes over to kld-transform-parser. If you have a minute, can you have a look if I'm following your process correctly? Everything seems to be linting, doc'ing, and running correctly. If everything looks good there, then I think we'll be good to go here too. Note that I included the |
I noticed that I can't run tests with |
To clarify, I'm trying this in the other repos I'm converting...haven't tried in this repo yet |
Re: npm test, that was due to a typo in your Re: esm, sure. If you prefer that to @babel/register, you might be able to change the test calls to Re: fully qualified types, IIRC, in this repo, it wasn't all working correctly for some reason, though maybe some types were ok or maybe it was due to some other reason--I can't recall. Yes, jsdoc does seem to be able to resolve types without the module prefix in at least some cases, but I'm not sure whether it is necessary to have it for the sake of certain relationships, e.g., when re-exporting the same content and when you wish to indicate this. FYI, there is a fresh new issue by the creator of jsdoc to improve on modules: jsdoc/jsdoc#1645 . (FWIW, I came across this issue through jsdoc/jsdoc#1533 which was asking for avoiding repeating all of the module paths within a module.) Typescript's approach is to use a kind of |
Re: Re: esm, I don't have a preference. I often put test scripts into a Re: JSDoc, OK, I'll just have to live with it for now. Back in the day, we had created scriptdoc to try and clean up these sorts of things, but, as we know, JSDoc won :) I've merged all of the other PRs from the other repos and will merge this now. Cheers! |
Did you try Thanks for all the merges! If you ever modularize kld-contours (and merge/publish the kld-affine/kld-polynomial development), I should be able to do a PR to simplify this PR routine (dropping commonjs)... |
I just tried replace |
Hmm... From the command line inside the root of
|
I'm on Node 10.11.0 (using nvm: 0.33.11) |
Oh, it interestingly only works with esm if within the scratch folder though... |
I guess go with whatever works best for ya... I think I may have seen this issue with @babel/register reported somewhere... |
@brettz9 I've pushed new versions of kld-polynomial, kld-affine, kld-contours, and kld-path-parser; all dependencies in this project. I think you mentioned that rollup and maybe even docs could be altered after that was completed. When you get a chance, please let me know what needs to be updated. Thanks! |
Checking things out now, atm, and at least affine and polynomial look good so far, will let you know on the others. Just wanted to mention you might want to add to the README, how to get the files, for each repo, e.g., whether:
import {Matrix2D, Point2D, Vector2D} from 'kld-affine';
import {Matrix2D, Point2D, Vector2D} from './node_modules/kld-affine/dist/index-esm.js';
const {Matrix2D, Point2D, Vector2D} = require('kld-affine');
<script src="./node_modules/kld-affine/dist/index-umd.js"></script> and var Matrix2D = KldAffine.Matrix2D; |
Yes, rollup can be altered (simplified) as per #38 though nothing to change as far as docs that I'm aware. |
I will make a cursory pass through the docs today to make sure I at least have minimal things in place, for example, installation and usage as you mentioned. Excluding updates to the README, do you feel this module is good to publish at this point? Once again I want to express my gratitude for all the help and time you've put into these module upgrades. I think these changes will have a positive impact on multiple axes ... and may even motivate me to give them more attention ;) |
Yes, it lgtm, and am eager to use it in my as-yet-incomplete project, https://github.com/brettz9/maptext . (Also appreciate your alerting me to the If it may be of interest, this maptext project is for annotating or viewing images representing text (e.g., original handwriting or calligraphy) with actual text, so that with text accurately aligned to image coordinates, one can search for text within an image (or collection of images) and have the relevant portions of the image(s) be highlighted so one can combine the advantages of text search with seeing an original or artistic work (similar to how Google Docs lets one search through OCR'd text, but this would be both open source and allow precise annotation). I hope to finish up this behavior soon. The project also uses the text info to allow users to drag a rectangle over the image to copy-paste the words associated within the marked-up regions one crosses. This is the reason for my interest in #20 , as I need to know not only if the rectangle intersects an existing text-mapped region, but if it might entirely contain it; for the rectangles and circles I'm currently supporting, I was able to hack my own solution, but if the ImageMaps library I'm using ends up supporting polygon annotations, I'd need something more sophisticated. |
Sounds like an interesting project. If I'm reading correctly, it sounds like you need to know if a hand-drawn rectangle or circle completely encloses another shape? If so, what kind of shapes would you be testing against? |
Just FYI, I added (back in) support for arcs. This is not highly tested but it is based on other code that has been working. I'll try to get the README somewhat updated and then push a version. After that I'll probably concentrate on docs and unit tests (in all repos). I am curious to try these new shiny modules in the browser though. It might be a good way to get my very old shape editor going again...which was used only for demo'ing and testing intersections because doing that from the command-line is not fun :) |
If by "hand-drawn", you mean mouse-driven, then yes. Editors can use the mouse to drag and resize an SVG rectangle or circle (and in the future, hopefully, polygons) over portions of text (e.g., over some original handwriting), and then associate Unicode text (i.e., the text comprising the handwriting) with that rectangle/circle/polygon. This allows users to drag a resizing rectangle over the same image (but without the editor-annotated SVG rectangles/circles being initially visible) so as to get text information (encoded by the editors) out of the image. I need to know which (if any) of the underlying SVG rectangle or circles (and hopefully polygons) that this user-driven rectangle encompasses or intersects (so that I can selectively reveal the outline of those underlying shapes and/or look up the text which is associated with those underlying shapes which interesect/are contained so that I can allow the user to copy Unicode text out of the intersecting/contained shapes the user has ended up highlighting). |
FWIW, you might consider using canvas for this. You can render the image to an off-screen canvas in one color with alpha, then render the shapes with the same color and alpha. Now all pixels that are darker then the original color must exist in both shapes. Use black with alpha so you only have to check one channel. There are other methods you can use if you want to know exactly which shapes are overlapping...food for thought. |
@brettz9 Just an FYI that I've pushed |
Excellent, thanks! This should motivate me to refactor my own code and see what I think of the APIs. |
Cool. As mentioned in our other thread, you can always create your own API; your own Shapes-like variant. There are 3 variants already, and I think at least 2 are useful :) |
This adds ES6 Modules in source and as a distribution format alongside UMD (which is just a wrapper for CommonJS, browser IIFE-wrapped global, and AMD).
I suggest looking at the diffs for the commits separately.
The first commit adds the least noise and is the meat of this PR. I've added the
dist
files to the repo as I find it can be helpful upon updates to ensure that one notices changes that are being made (or not being made) before releases. However, if you want, I can ignore these files in.gitignore
(though of course not in.npmignore
). (FWIW, if you ever convert yourkld-affine
andkld-polynomial
package dependencies into ES6 Modules, we can avoid the Rollup commonjs plugin.)The 2nd commit just takes advantage of the fact that Babel was added and converts your constructors to ES6 classes (because of the indenting differences, there is more noise in this diff).
The third commit uses jsdoc's modules to ensure that types are interlinking properly, and though there are a good number of changes, is pretty straightforward.