types for atom/tool-bar

[aminya]

https://github.com/suda/tool-bar

See this in action here: https://github.com/aminya/juno-plus/blob/78c1ee36c83cc4f9371d7dafd0498b3ccec76aa1/src/juno-plus.ts#L1

---

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If adding a new definition:

• The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: <>

[typescript-bot]

@aminya Thank you for submitting this PR!

🔔 @GlenCFL @smhxx @lierdakil - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

[aminya]

Something minor: I thought addButton return is void, however, it returns the button element. I need to adjust this PR for that.
https://github.com/DefinitelyTyped/DefinitelyTyped/pull/43727/files#diff-189074f422e48d631fa1eb716c639a6aR144

[lierdakil]

I'm a little torn on this one. On the one hand, tool-bar is a third-party package, and neither myself, nor, to my knowledge @GlenCFL or @smhxx are using it anywhere, so maintainers won't be able to actually maintain definitions for it in any meaningful sense. On the other hand, saying "it's a third-party package so we can't maintain it" isn't right either, since we do have definitions for linter included (well, to be fair, it is a kinda-sorta part of atom-ide, which was at one point quasi-official)

In general, I would suggest making a separate package for this, but the definition probably references stuff from base atom definitions, which would make this approach more than a little unwieldy.

@aminya -- perhaps the solution here is to add yourself to maintainers for the atom type definitions.
The definitions could certainly use more eyes, and this would solve the problem of "maintainers not using the package they are supposed to maintain" wrt tool-bar. That said, I would suggest waiting for input from other maintainers first (if any).

@GlenCFL @smhxx thoughts?

[aminya]

How can I add myself as a maintainer? That's totally fine to me.

[GlenCFL]

My intent in adding the type definitions for third-party packages was always for
additional ones to be added, so long as they're both correct and potentially useful.
There's not much downside in them existing, as they rarely change and thus introduce
no maintainability issues. The three existing ones were last modified two years ago and are
still valid, with only a few minor additions being missing. The verification concern is valid, as documentation cant always be trusted.

@aminya You'd just add yourself to the list at the top of the Atom definition file.
My only issue with the PR is the use of class instead of interface for the ToolBarManager type, as this can result in invalid code if someone imports this type directly from the module and tries to instantiate an instance of it.

[aminya]

My intent in adding the type definitions for third-party packages was always for
additional ones to be added, so long as they're both correct and potentially useful.
There's not much downside in them existing, as they rarely change and thus introduce
no maintainability issues. The three existing ones were last modified two years ago and are
still valid, with only a few minor additions being missing. The verification concern is valid, as documentation cant always be trusted.

Thank you! I will do it.

@aminya You'd just add yourself to the list at the top of the Atom definition file.
My only issue with the PR is the use of class instead of interface for the ToolBarManager type, as this can result in invalid code if someone imports this type directly from the module and tries to instantiate an instance of it.

ToolBarManager is a valid class in tool-bar: https://github.com/suda/tool-bar/blob/0b8a1ced016b852ed175f72d7a860b324ef438fd/lib/tool-bar-manager.js#L4
Although not exported in the end: https://github.com/suda/tool-bar/blob/master/lib/tool-bar.js

[GlenCFL]

We're defining a module, atom/tool-bar, where that class isn't actually being exported.

import { ToolBarManager } from "atom/tool-bar";
const toolbar = new ToolBarManager();

That's a runtime error introduced through the definition file.

[lierdakil]

ToolBarManager is a valid class in tool-bar

The primary difference between class and interface is that classes can be used in new expressions. The policy we use, at least for core Atom definitions, is "if you can't do new X in client code, it shouldn't be declared as class". There's a good reason for this policy actually, since this is valid code as far as TypeScript compiler is concerned:

declare class SomeRandomClass {
  constructor();
}

const newClass = new SomeRandomClass();

but the produced javascript will break, because constructor for SomeRandomClass isn't actually in scope.

As far as I can tell, it's completely impossible to get at the constructor for ToolBarManager in the client code, but declaring it as class will allow new ToolBarManager as far as TypeScript is concerned (but it isn't actually valid). Hence, it's better to declare it as interface.

Does this make sense?

[aminya]

Yes! Thank you all. I will change it to interface!

[aminya]

I updated the types. Could you approve this?

[typescript-bot]

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

[armanio123]

@GlenCFL, @lierdakil, @aminya I didn't quite follow the conversation, is this good to merge?

[aminya]

@GlenCFL, @lierdakil, @aminya I didn't quite follow the conversation, is this good to merge?

Yes. I updated the types and GlenCFL already approved it.

[typescript-bot]

I just published @types/atom@1.40.2 to npm.