TypeScript

[aminya]

Ready to get merged - Tested and is working

Description of the Change - Release Notes

• Converting codebase source to typescript while being completely backward-compatible
• Adding type definitions + documentation for API
• 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]

@maxbrunsfeld Could you review this?

[aminya]

Is there anyone to check this?

@nshikov @maxbrunsfeld @t9md @50Wliu

[aminya]

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.

[winstliu]

Mostly minor stuff, but still a few questions

[winstliu]

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.

[winstliu]

And: is this meant to be public?

[aminya]

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.

[winstliu]

Buncha small stuff. Only thing I'm blocking over is the last TODO comment.

(p.s. you'll do reviewers a favor if you elaborate more on what you mean by your TODOs in the future 🙂)

[aminya]

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]

Both :)

[winstliu]

I'm happy with this 👍.

[aminya]

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

[winstliu]

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