atom / atom-select-list

An etch component to show select lists with fuzzy filtering, keyboard navigation and other cool features.

MIT License

33 stars 33 forks source link

TypeScript #28

Closed aminya closed 3 years ago

aminya commented 4 years ago

Ready to get merged - Tested and is working

Description of the Change - Release Notes

[X] Converting codebase source to typescript while being completely backward-compatible
[x] Adding type definitions + documentation for API
[x] Optimizing the code wherever possible

Verification Process

I verify the changes against other packages that use atom-select-list: for example, atom-indent-detective uses atom-select-list.
The tests are untouched to show backward compatibility

Possible Drawbacks

Nothing (backward compatible - zero change in the API and JavaScript code)

Alternate Designs

using @types/atom-select-list. But this doesn't allow us to use the power of typescript for optimizing and catching the bugs.

aminya commented 4 years ago

@maxbrunsfeld Could you review this?

aminya commented 4 years ago

Is there anyone to check this?

@nshikov @maxbrunsfeld @t9md @50Wliu

aminya commented 4 years ago

Could you check this PR? @lkashef @smashwilson

How can I become a member of the Atom organization? I have at least 5 PRs that are open and no one reviews them. I can take responsibility for these and future potential inquiries.

aminya commented 3 years ago

Disclaimer: I'm no longer an Atom maintainer and my knowledge of the ecosystem is now 1.5 years old. That said, still happy to give reviews that pop up on my notifications feed when I have the time.

Glad to hear from you!

This looks like a fairly straightforward port to Typescript. Have the current maintainers indicated that they're open to switching to Typescript? (I think there were some conversations about this...too fuzzy for me to recall for sure though)

There are only 2-3 people officially working part-time on Atom. They rarely get time to address the PRs. We are considering forking the whole organization in @atom-community, so I am not surprised anymore that I did not get any response here.

Most of my comments are related to the configuration of Typescript itself, plus a few questions about the TODO comments you left.

I addressed most of them. I left some comments for the unresolved ones.

aminya commented 3 years ago

I looked through the code to re-familiarize myself with this. It technically is assigned in the constructor, via computeItems. Does Typescript throw an error if it's not explicitly set in the constructor itself? I forget how good it is at following the codeflow.

I'm not positive, but I think it is necessary because of the slices we do on it; this way this.props.items is the original copy and this.items is the filtered & ordered list that's shown.

And: is this meant to be public?

Yes, that seems like it. I will change it to private because it is not documented.

aminya commented 3 years ago

Per atom/etch@18512fc/lib/patch.js#L46, it looks like {} is the default value that's passed in if there's no new props. Which says two things:

The = {} shouldn't even be needed b/c we should never be passed an undefined value, per the source above Since it's intentional that we can be passed an empty object, I think it's fine that we just handle that normally. So I don't think this TODO needs to stay.

Which one should I remove? = {} or the TODO?

winstliu commented 3 years ago

Both :)

aminya commented 3 years ago

@50Wliu Could you merge this if you have access? I don't want to wait another year for merging 😞

winstliu commented 3 years ago

Sorry, I don't merge things anymore :/. @sadick254 is this something in your area?