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

ES6 modules #37

Merged
merged 3 commits into from
May 6, 2019
Merged

ES6 modules #37

merged 3 commits into from
May 6, 2019

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented May 5, 2019

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 your kld-affine and kld-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.

@brettz9 brettz9 mentioned this pull request May 5, 2019
@brettz9 brettz9 force-pushed the es6-modules branch 3 times, most recently from d44ea02 to cb7a2a1 Compare May 5, 2019 07:10
…` `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
@thelonious
Copy link
Owner

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:

 *  @param {module:kld-intersections.Point2D} p3
 *  @returns {module:kld-intersections.IntersectionArgs}

The Point2D reference wasn't too surprising, but the IntersectionArgs reference was. I would expect JSDoc to be able to resolve all types within the module. It would be nice to be able to use the bare type without the module prefix...lots of typing

@thelonious
Copy link
Owner

thelonious commented May 5, 2019

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 esm module so I can run test scripts (in the scratch folder) via node -r esm <script-name>. That works really well.

@thelonious
Copy link
Owner

I noticed that I can't run tests with npm run test. I'm running node v10.15.3

@thelonious
Copy link
Owner

To clarify, I'm trying this in the other repos I'm converting...haven't tried in this repo yet

@brettz9
Copy link
Contributor Author

brettz9 commented May 6, 2019

Re: npm test, that was due to a typo in your .babelrc files (though I'm not sure why @babel/register needs to pick up the file and/or the preset-env to support module parsing, but it apparently does). Fixed in the two PRs (kld-path-parser and kld-transform-parser).

Re: esm, sure. If you prefer that to @babel/register, you might be able to change the test calls to mocha --require esm and drop the @babel/register devDep (and maybe the @babel/core I recall was also needed by @babel/register), though you'd need to test to confirm. But FWIW, you can also use @babel/register for your scratchpad files, e.g., node -r @babel/register <script-name>.

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 import statement within the jsdoc statements, making for an intuitive approach which it seems jsdoc might adopt.

@thelonious
Copy link
Owner

Re: .babelrc, Yes, I frequently swap el and le. I've merged those requests and can confirm that I'm able to run tests now.

Re: esm, I don't have a preference. I often put test scripts into a scratch folder. I exclude those from linting and docs. I also add a shebang and make them executable for my convenience. I noticed I wasn't able to run those from a shell, but using #!/usr/bin/env node -r esm along with the esm module fixed that. Is there an equivalent I can use with the babel setup?

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!

@thelonious thelonious merged commit c6b6136 into thelonious:development May 6, 2019
@brettz9
Copy link
Contributor Author

brettz9 commented May 6, 2019

Did you try node -r @babel/register <script-name>? That works for me on your scratchpad scripts...

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)...

@brettz9 brettz9 deleted the es6-modules branch May 6, 2019 03:11
@thelonious
Copy link
Owner

I just tried replace esm with @babel/register one of the kld-transform-parser scripts and that did not work. I even tried the full incantation from the command-line and got the same error. It may be user error on my part.

@brettz9
Copy link
Contributor Author

brettz9 commented May 6, 2019

Hmm...

From the command line inside the root of kld-transform-parser, the following are both producing output (and the same output as with "esm"):

node -r @babel/register scratch/dump_lexemes.js
node -r @babel/register scratch/parser_test.js

@brettz9
Copy link
Contributor Author

brettz9 commented May 6, 2019

I'm on Node 10.11.0 (using nvm: 0.33.11)

@brettz9
Copy link
Contributor Author

brettz9 commented May 6, 2019

Oh, it interestingly only works with esm if within the scratch folder though...

@brettz9
Copy link
Contributor Author

brettz9 commented May 6, 2019

I guess go with whatever works best for ya... I think I may have seen this issue with @babel/register reported somewhere...

@thelonious
Copy link
Owner

@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!

@brettz9
Copy link
Contributor Author

brettz9 commented May 7, 2019

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:

  1. For bundlers:
import {Matrix2D, Point2D, Vector2D} from 'kld-affine';
  1. For ESM in the browser (modern browsers only)
import {Matrix2D, Point2D, Vector2D} from './node_modules/kld-affine/dist/index-esm.js';
  1. In Node:
const {Matrix2D, Point2D, Vector2D} = require('kld-affine');
  1. In older browsers
<script src="./node_modules/kld-affine/dist/index-umd.js"></script>

and

var Matrix2D = KldAffine.Matrix2D;

@brettz9
Copy link
Contributor Author

brettz9 commented May 7, 2019

Yes, rollup can be altered (simplified) as per #38 though nothing to change as far as docs that I'm aware.

@thelonious
Copy link
Owner

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 ;)

@brettz9
Copy link
Contributor Author

brettz9 commented May 8, 2019

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 static keyword which had somehow escaped me as being available now in JS.)

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.

@thelonious
Copy link
Owner

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?

@thelonious
Copy link
Owner

thelonious commented May 8, 2019

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 :)

@brettz9
Copy link
Contributor Author

brettz9 commented May 8, 2019

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).

@thelonious
Copy link
Owner

thelonious commented May 8, 2019

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.

@thelonious
Copy link
Owner

@brettz9 Just an FYI that I've pushed v0.5.0 to NPM. I expect to be making more changes over the next week or two, but this is the first release with all of your hard work.

@brettz9
Copy link
Contributor Author

brettz9 commented May 9, 2019

Excellent, thanks! This should motivate me to refactor my own code and see what I think of the APIs.

@thelonious
Copy link
Owner

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 :)

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.

None yet

2 participants