-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Remove TypeScript #3
Conversation
This seems like a good idea! I would just try updating the babel deps within the 7.x semver, check the transpile with newer babel, and see if the generated JS code gets any nicer? Since for all I know there may be years of babel 7.x updates since this repo was last updated. But there's a large I'm thankful this is a very small repo. I will try to actually read all the meaningful diff, since it's not that many lines. |
@DeeDeeG To clarify, I actually didn't use much of any generated output, as it was mostly quite ugly. I only used the generated output for how the module was being exported, and handled requires. But I'll take a look and see if I can find any nicer way to do those steps, but otherwise it may just stay the same. Especially since there's only about 30 lines of diff in the actual source code here. But I'll see what I can do |
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.
Okay, I thought the output JS might be funkier from the description in the PR body, but this is a very easy diff to read.
While I admire the original authors' use of TypeScript for type safety, and it seems an appropriate application of some type safety when interacting with a data format as arcane as diffs and git porcelain stuff, not so many people in Pulsar org like or are comfortable with TypeScript. And the amount of complexity dropped from this repo is a really large percentage of the repo, by line count.
I mean no disrespect to code authors who have moved on. And meanwhile I think this is the right move for this fork in this org at this time. (I personally would not know how to deal with the TypeScript aspects of this codebase, and it would be a barrier to making changes. For instance.)
Thanks for doing this @confused-Techie !
@DeeDeeG Thanks a ton for the review, and approval! But yeah my description, and the huge diff count is a bit misleading for what's actually changed here. But otherwise, you are right, that TypeScript makes the most sense when dealing with outside data like this, but at the same time, it is very clear how much complexity has been dropped by removing it here. Especially now with the ease that we can install this repo from any sha and not have to worry about any build steps. But I may be biased always preferring native JS, and not having to have any build steps at all in code. But exactly, no disrespect to the original authors, this just makes the most sense in how we operate on this org. But with your approval I'll go ahead and merge, thanks! |
Apologies about the diff of this PR being so large.
This PR does the following:
While I normally am very careful about any code conversions to both the specs and source, I felt they were not at all needed this time, because the only things that changed in the specs was the extension, and
require
ing it's modules. Then the only thing that was changed in the source to do this was removing Type annotations within the code, changing fromimport
torequire
, and the way each function was exported.The new method of exporting, while strange to see in JavaScript, was done to match the exact way it's done in the
dist
JavaScript code generated after converting to TypeScript.But hopefully GitHub's diff UI can help drive home just how small these changes actually are.
But since we could, I then removed the vast majority of now useless deps from the repo. Which happily make this whole codebase significantly simpler.