Add typescript typings directly into shelljs repo

[voy]

Currently there is a separate @types/shelljs package, which is even slightly out of date. E.g. it does not contain the verbose configuration option etc. Would anyone be against adding the type information directly into this package (as is the recommended way - see publishing).

In case there is support for doing this, I would be happy to reach out to the typings author to ask for permission and then make the necessary updates.

[voy]

Any thoughts on this, @nikeee?

[nfischer]

I don't know much about typescript types, so I don't think I could adequately review/maintain it.

I'd be more than happy to work with the maintainer to get things up-to-date though.

[voy]

I will of course not object to things being done for me, but this being open source you can just let me handle this one and focus on other things.

[nikeee]

Sounds good!

[voy]

Ok good, I will take a look at it and leave your name as author in the typings file. Hope that is enough as attribution, @nikeee.

[nfischer]

Where do the types exist currently? @voy @nikeee are you two the maintainers?

[voy]

The typings are currently available from DefinitelyTyped, which - as far as I understand it - is mostly a legacy thing. From there they are exported as a @types/shelljs package, which is the second best solution to actually bundling typings with the library itself. Bundling is best, because that way typings are versioned as expected along with the code they provide types for.

As I said before, the typings are now slightly out of date, not providing types for some configuration options etc. Therefore, I think we should do the right thing and bundle the updated typings directly with the library itself.

[nfischer]

What are the alternative options? I hesitate to merge into this repo because I don't want to take up the full maintenance burden.

Is it feasible to move it into a separate repo under the shelljs org, with you and @nikeee as co-maintainers along with the rest of the org?

[nikeee]

Maybe. The definition file(s) only has to be packaged and published together with the rest of the npm package.

[nfischer]

he definition file(s) only has to be packaged and published together with the rest of the npm package.

Could you explain that for me? The typing info doesn't travel that way currently. What benefits does making the change bring? What are the consequences to packaging independently?

I envision we would specify that shelljs@v0.8.x corresponds to shelljs-types@v0.8.y. So upgrading one would require upgrading the other.

[voy]

I think @nikeee wanted to suggest that we could hold the definitions in a separate repo and put them together as a build step, if I am not mistaken. Not sure how that would solve the ownership problem though, at that point it seems same as bundling them together.

@nfischer you could of course do that, but it is a markedly worse developer experience. You have to spend extra cycles on making sure the types package is available at the same version and the end user has to actively look for that package. They also have to make sure that at any given moment they're using compatible versions of the two packages. Having types in the same repo is clearly superior in both of these regards and provides the best possible of the box experience.

In short, while I understand it is extra burden, I suggest to opt for the best possible solution even if it means more work. I think we can step up as a community and help you updating those types. In the end even if types exist completely separately I am afraid I know where users will go if they ever go out of date (issues here).

[nfischer]

Yeah, I see those benefits. On the other side, here are the drawbacks to keeping it in the same repo:

• If external developers contribute to shelljs, they also have to update types. This may be burdensome for non-typescript developers. Perhaps maintainers can help out here.
• If we (the maintainers) get lazy and let types fall out of date, we have to delay the release to fix them. This is good for the type info, but slows the rest of the package down.

If we do decide to add typescript info into the repo, then we need to add official support for typescript. What would it take to get CI running the latest version of the typescript compiler?

[voy]

@nfischer I think getting TS running in a CI pipeline would not be difficult, I have a project written completely in TS and it is running tests flawlessly. The question would what tests we can run. I imagine we could put together a sample project with calls to shelljs APIs and have it compile against the typings. It will only be as good as the sample project, however. Did you have anything else in mind?

[nfischer]

Would it be possible to add an extra CI run of our test suite to compile everything with tsc and run the tests?

I would expect tsc to be a NOOP initially. It'll be more useful once we add types into the repo.

[voy]

Can we get to some sort of a closure on this? I do not want to be mean in any way, but the typings are ~500 lines, most of it comments. I think they require only occasional updates (unless there's a big breaking change) and updating them is not too difficult. A good opportunity for contributors, I would say. Quite honestly, I think we have spent so much time discussing in this thread that the work required could easily have been just done. The typings are anyway floating out there and as evidenced by this thread, people will come here if they become out of date. It would have been so much easier for me to just send a PR in the first place.

[nfischer]

I'm not objecting to this, I'm not sure what response you were waiting for.

Steps for implementation:

1. Provide support for typescript by running our CI under tsc.
2. Move typings into ShellJS.
3. Update typings for latest version of our API.
4. Include typings when running our test suite under typescript. This verifies correctness and ensures types are up-to-date.

[Strum355]

Any news on this? @​types/shelljs seem to be pretty old, as they tell me i can only pass a callback to echo('text').exec(...) ...

Edit: for those that may come here in the future, i drafted up a very rough replacement, please use carefully

exec(command: string, options?: {async?: boolean, silent?: boolean, encoding?: string, shell?: string}, callback?: ExecCallback): child.ChildProcess

[nfischer]

@Strum355 I'm in favor of bringing typings into shelljs itself, but I need help setting up our infrastructure to actually test that code. Like I've said, I'm not a typescript user, so I need help setting this up with our travis/appveyor configs. This is basically step 1 that I outlined in #736 (comment).

[natepisarski]

I just wanted to leave a comment to let you know that this would be a greatly appreciated feature. I totally hear what you've said about this over the past 2 years, how it increases the maintenance burden and raises the barrier of entry for contributors. The thing about typescript is that it's very easy to tell what's going on, though.

Without being a typescript developer, seeing the function:

function useNumber(value: number): number { return 5; }

is pretty obvious. Even uncommon, decently advanced types like discriminated unions are simple enough to understand, and I personally think is easier to understand with 0 typescript background than a function with no type hints.

function getProcessRunningState(): FinishedProcess | RunningProcess | NewProcess

And now, years after the issue was created, we're starting to see that typescript isn't going anywhere. We might even be heading to a place where it's industry standard. There are types for like half of all npm packages, including the big libraries like React. Editors are integrated tslint more and more, etc.

[nfischer]

@natepisarski I'm open to reviewing a PR.

That said, after looking at the definitelytyped repo, I see some mistakes (e.g., cp() returns a ShellString, not a void). I'll accept typings as-is, but someone more experienced than me should follow-up with a fix.

That said, I can't realistically maintain these typings unless we have infra. If we can't land infra, this would be maintained by community patches. But, I don't think this is any worse than the current state?

Also, if someone sends a pull request to copy the typings, please also include a copy of the definitelytyped license as per "The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software."

[ristomatti]

Updated link to DefinitelyTyped typings: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/shelljs/index.d.ts