Convert this project to TypeScript#325
Conversation
|
This PR is blocked on goto100/xpath#116. |
You dont need to wait for them to update xpath. Just add your own typings |
I suppose I could... then remove them later... |
|
Maybe we should wait after all. @cjbarth has been working on this full on for a while, and might need a breather |
Codecov Report
@@ Coverage Diff @@
## master #325 +/- ##
===========================================
- Coverage 82.88% 72.44% -10.45%
===========================================
Files 7 9 +2
Lines 777 860 +83
Branches 0 228 +228
===========================================
- Hits 644 623 -21
- Misses 133 136 +3
- Partials 0 101 +101
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
LoneRifle
force-pushed
the
ts-convert
branch
3 times, most recently
from
191c6e9 to
d316253
Compare
|
This PR has taken quite a bit more than anticipated to get right. I would appreciate feedback at this time. I hope to get this landed and then have follow-on PRs to address better typing and refactoring. This should get us in a good direction though. |
|
Is it fair to say that SignedXml has most of the changes? |
Yes. Would it make it easier if I did a rename-only PR ahead of this, even though it would break the build, just so that this PR's diff is easier to read (and which would then fix the build)? |
I merged in a rename-only PR so that this one has much better diffs. LMK how else I can help the review go better on this. |
LoneRifle
left a comment
LoneRifle
left a comment
There was a problem hiding this comment.
lgtm otherwise. thanks for tirelessly undertaking this task to improve code hygiene here!
I think there are a few minor issues and nits to polish through, and then we should be good to go.
src/index.ts
Outdated
| export {} from "./utils"; | ||
| export {} from "./types"; |
There was a problem hiding this comment.
? any reason why these lines are explicitly defined?
There was a problem hiding this comment.
The idea is that a consumer of this package shouldn't have to know the innards of this library to use the things from those files. They will all appears as members of xml-crypto in their include or requires statement.
src/utils.ts
Outdated
| buffer = Buffer.from(digest, "base64"); | ||
| expectedBuffer = Buffer.from(expectedDigest, "base64"); |
There was a problem hiding this comment.
Buffer.from was only introduced in Node 5.10 though. We should decide whether we want to keep these since we now only support much later versions of Node.
I'm personally in favour of dropping these especially given that we've moved to TypeScript, and if someone really wants to, they can figure a way to get from TypeScript to transpiled JavaScript.
This also allows us to remove the need to check the major version of Node at runtime.
There was a problem hiding this comment.
I'm not sure what you're saying here. I think I made this change (probably out of scope) to remove a deprecation warning. Are you proposing that we revert this or keep this?
There was a problem hiding this comment.
I'm proposing to revert this or to remove it altogether.
If we were to keep this, it would trigger a runtime error, if running on Node 5. That is why there is a check for Node major version earlier in the code path; to determine if Buffer.from is present. As it is, both branches of the conditional are largely the same.
I'm leaning towards removing this if block altogether and simply replacing with Buffer.from lines.
There was a problem hiding this comment.
I've made the change, but I'm also wondering the history of the lines below this with the comment "Compatibility with Node < 0.11.13" Does that need to be removed? Honestly, it looks like a reasonable check to me no matter the version of Node.
LoneRifle
left a comment
LoneRifle
left a comment
There was a problem hiding this comment.
lgtm - thanks! I look forward to a fresh start with this package!
3 participants
Please, @LoneRifle and @shunkica, feel free to push to this branch, or create a PR against it if you can't push, as often as you'd like to help get this done more quickly.