-
Notifications
You must be signed in to change notification settings - Fork 36
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
build: migrate to typescript #296
Conversation
@raveclassic Thank you for this incredible undertaking! The core team discussed briefly today, and we are all supportive of the move to TS. I should have time to start looking at this over the weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it's taken me longer to get started reviewing this than I thought. Thank you again for this work! I left a couple higher level items for discussion. I'll try to make incremental progress in reviewing over the next couple days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress. Let's keep this going! A few more comments as I work my may through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more reviewed files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
25% reviewed. Phew!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slowly but surely, I'm working my way through. Thanks being patient and continuing to work through this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PropagateTask.ts reviewed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whew! I think I made it all the way to the end 😄 Thanks for continuing to move this forward! A few more questions and comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great overall. A couple more very minor comments.
Hey @raveclassic, I haven’t forgotten about this. I’m on vacation with little to no internet access until 7 Oct. I’ll get back to this ASAP once I’ve returned. |
Yea, @raveclassic, I haven’t forgotten either. Just very busy with an upcoming release at my day job, which have been stealing my energy for extra code and open source projects. I will also be back on track next week. Thanks for your patience. |
No problem, guys, I'm also on vacation right now until next week :) |
I am delayed. |
Guys? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raveclassic @Frikki I finally got back to this 😬
a223543
to
5d638a3
Compare
# Conflicts: # package-lock.json
Now that the @raveclassic Are there things you know are still left to do? If so, could you edit the PR description and list them (with checkboxes!). If not, then maybe we just need a final review and for at least one of the core team to pull a clean copy and ensure everything compiles and type checks. Other thoughts on how to proceed? |
@briancavalier It seems like everything you and @Frikki mentioned is resolved. Do you remember what files/packages you haven't seen yet? |
@raveclassic Thanks. I think I've been through all the files at one time or another. I feel like I need to do 2 things:
I'll start on those now. |
I just did this and everything worked perfectly, except for the perf tests. Running the various test files directly with I think it's an easy fix: We can update the package.json scripts from, e.g., Now I'm on to a final skim review of all the files. |
return | ||
} | ||
this.sink.event(t, indexValue.value) | ||
} | ||
|
||
_dispose (t, index) { | ||
private dispose(t: Time, index: number): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making all the "private by convention" methods actually private .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it through a quick scan of all the files again 😌. Looks good to merge!
Thank you for this monumental effort and your patience while we all worked through it together, @raveclassic. Great work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that now that tests are passing, we should get this PR into production.
@briancavalier Turns out this has never been released :D Could you publish the new version? |
Hey @raveclassic. Whoops! Thanks for the nudge. Just published updated versions of everything:
|
@briancavalier i get a Typescript transpiling error because of a wrong reference to the entry TS file within package.json check PR #525 |
Hi, guys!
So following mostjs/hold#36 this PR moves all the source entirely to typescript. There're a lot of changes and I thought although it would be hard to review everything in one time it would be even harder to migrate separately.
Some notes about the changes:
Sink#end
method and so on - so I removed them.d.ts
files may be different from originalmost.d.ts
although I tried my best to replicate everything. However I may have missed something.package.json
. If it's important I'll try once again to migrate them as well.I don't expect this to be merged smooth but I'm looking forward to finding out all the problems and fixing them. I do love
most
and I would love to move this awesome library to the best static-typing solution.