Add TS typings + build improvements #54
Conversation
|
Awesome. I'll take a look! |
|
Give me a couple minutes, I didn't notice that Travis failed. I was futzing with the rollup config for terser and didn't re-run tests afterwards. (The good news is that apparently the tests do use the build artifact, which I should have noticed. Sorry!) |
|
No worries :-) |
|
OK, the commit I'm pushing now uses |
mgrs.js
Outdated
| @@ -286,10 +287,10 @@ function UTMtoLL(utm) { | |||
| /** | |||
| * Calculates the MGRS letter designator for the given latitude. | |||
| * | |||
| * @private | |||
| * @param {number} lat The latitude in WGS84 to get the letter designator | |||
| * @private // TODO: is this actually private? If so, why is it exported? | |||
There was a problem hiding this comment.
good catch! that line should be deleted. I recently made it public (i.e. exported it), so that I could test it: https://github.com/proj4js/mgrs/blob/master/test/test.js#L123
There was a problem hiding this comment.
Well, there's public, then there's public. If you don't think it warrants showing up on the public API I don't think there's any problem with leaving @private in the JSDoc. Typescript has an @internal annotation that works in a similar way, that excludes it from generated typings.
There was a problem hiding this comment.
Let's keep it the way that is now then. I'm worried about making the API more complicated without a clear use case. If a clear use case for people using getLetterDesignator comes up independent of forward or inverse, I'll change my mind :-)
Could you replace the TODO with // hiding from the public API to keep things simple for users or something to that effect?
package.json
Outdated
| "pretest": "npm run clean && npm run build", | ||
| "test": "nyc mocha test/test.js", | ||
| "clean": "rimraf dist", | ||
| "build": "mkdirp dist && rollup -c" |
There was a problem hiding this comment.
Could you provide more context on why the switch to mkdirp from mkdir -p? Is there any alternative to mkdirp? Unfortunately, it doesn't look like the project is still being maintained: https://www.npmjs.com/package/mkdirp
https://www.npmjs.com/package/mkdirp
There was a problem hiding this comment.
mkdir -p on Windows makes a directory called "-p". I wish I were kidding.
I didn't bother researching to see if there's a maintained project for this, and honestly it's a tiny chunk of functionality so it might be simpler to just write a 4-line script that does it using fs methods, but I don't know of a cross-platform way to do this using shell commands.
There was a problem hiding this comment.
I think we can just change mkdirp to mkdir and removed the mkdirp dependency. I don't think the -p is necessary. Let me know if I missed anything!
There was a problem hiding this comment.
I replaced mkdirp with mkdir. Let me know if this doesn't work for some reason.
|
I made just a couple comments. Other than that, looks good to go! |
|
Hopefully that clears things up, if you want to make any changes (or just revert any of the "fixes" I put in) please feel free. |
|
Awesome! Thanks for the quick feedback :-) |
|
FYI that change I made is from the GH in-browser text editor. Since you're a maintainer here and I set the PR to "allow edits", you should also have permissions to do that. (I don't mind doing it, I just wanted to make sure you were aware.) |
Hi @DanielJDufour , I came to add TS typings and wound up fixing a few build issues as well. I tried to make each fix as its own commit so you can examine them separately. I can also make separate PRs if you'd like, but the overall scope is still pretty small so I thought it might be OK to do as one.
I've tested the reconfigured build and it works in my consuming applications, but I didn't add a "smoke test" or anything automated that uses the build artifact.