node-slack-sdk icon indicating copy to clipboard operation
node-slack-sdk copied to clipboard

Move non-exposed `@types/*` dependencies to "devDependencies"

Open clavin opened this issue 4 years ago • 6 comments

Description

In #799, a discussion unfolded about the trade-offs of moving @types/* dependencies that don't contain exposed types to "devDependencies". A previous related dicussion can be found in #514.

In favor
  • Developers who consume these packages don't care about or need @types/debug to consume @slack/events-api. This is just an example, but it points out the disconnect between these "dependencies" and each package's types.
  • Minor bundle size difference.
Against
  • Determining which @types/* goes where is a complex process that involves understanding what is and isn't exported by each package's API.
  • It's relatively simple to make a small change that bubbles up to the API layer, exposing a type from a package that wasn't previously exposed. That is, it's too easy to get this wrong.
  • A solution implies more code to maintain.

Proposed solution

The generated .d.ts files include triple-slash directives (/// <reference ... />) that tell the TypeScript compiler what @types/* packages are needed for that .d.ts file. The set of @types/* packages referenced by the .d.ts files for a given package could be reconciled with those listed in "dependencies" of that package's package.json. If any are missing or any extra are present, there's an error.

This could be implemented as an integration test or a post-build tool. The algorithm would look like:

  1. For every package in the root directory packages/:
    1. Read package.json as packageJson
    2. Create a Set deps containing:
      1. Every dependency in packageJson.dependencies starting with @types/
        1. The @types/ prefix should be removed when adding to deps, e.g. the dependency @types/node would be added as node
    3. Create a Set refs
    4. For every file ending in .d.ts in that package's dist/:
      1. Read the contents of that file as a string fileContents
      2. Split fileContents into lines (split by \n)
      3. Filter lines of fileContents to include only those starting with ///
      4. Map each line to the match of that line on the regular expression types="(.+)"
      5. Filter lines to exclude non-matches
      6. For each remaining match ref (which is a string like node or debug or express):
        1. Add ref to refs
    5. If the set difference of deps without refs is not empty:
      1. Any values in the difference are dependencies that should be moved to devDependencies
    6. If the set difference of refs without deps is not empty:
      1. Any values in the difference are dependencies that should be present in dependencies. Either they are missing or wrongfully in devDependencies.

The "result" of a script like this could be anything:

  • It could issue warnings to the console to alert the developer that dependencies need to be moved
  • It could make the changes itself, either by moving dependencies between dependencies and devDependencies (and vice-versa) or by adding the latest version of the types to dependencies.

What type of issue is this? (place an x in one of the [ ])

  • [ ] bug
  • [x] enhancement (feature request)
  • [ ] question
  • [ ] documentation related
  • [ ] testing related
  • [x] discussion

Requirements (place an x in each of the [ ])

  • [x] I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • [x] I've read and agree to the Code of Conduct.
  • [x] I've searched for any related issues and avoided creating a duplicate issue.

clavin avatar Jul 17 '19 19:07 clavin

The common solution i've always seen is the logical:

  • Consumed types are dependencies, just like consumed dependencies are
  • Build-related/tooling types are dev dependencies

They are no different from regular dependencies.

If a class in the sources (i.e. not tests) depends on a type from a @types package, that package is a regular dependency in the same way that if it depends on an actual class from a package, that package is also a regular dependency.

You'd only add types packages to dev dependencies if they are for things like tests which are not exposed in the consumed package (i.e. they don't get used in the sources).

43081j avatar Jul 18 '19 14:07 43081j

🤔 I don't think that just because something is in the original source code indicates it should be a regular dependency. For example, take documentation tools that read JSDoc comments. Documentation comments are scatted throughout source code (and often generated/built source code too!). But just because the documentation comments are in the source that downstream packages consume doesn't mean that the tool used to create documents from that generation should be listed in dependencies; that tool isn't needed for your source code to be interpreted or run by downstream packages.

I see TypeScript in the same light. The source for most packages in this repo is written in TypeScript; however the typescript package is not a dependency of any of those packages. It's a devDependency--even though we also distribute .d.ts files! That's because none of our packages rely on the typescript package after the source code is built.

This should extend to types. After building .ts files, we end up with a bunch of .js and .d.ts files in a dist/ directory in each package. Some of those .js files depend on other packages via import or require, and some of those .d.ts files depend on certain typings via triple-slash reference directives (/// <reference ... />). Some of the tools used while building (typescript) are no longer needed, so we don't ask package consumers to download and store them; this could easily be extended to @types/* packages. Even though some @types/* are referenced and used in the original source, the built .d.ts files never refer to them, so we shouldn't ask package consumers to download and store them--they aren't used after the build stage.

clavin avatar Jul 18 '19 17:07 clavin

I think an important thing to realise here is that the types are paired with the modules they represent.

I can see your point of view but a language feature isn't the same as something external tooling parses (jsdoc).

Imagine for a moment that all types for packages existed alongside their package. This is the way it should be, the types repo is a workaround for the fact not all packages ship types.

So anyway, imagine every package consists of the source and the types. If you import something from that package, like a class, you now depend on it.

If you import a class in a test, you now depend on that package as a dev dependency too.

Types and source are a pair and should be seen as such. They are meant to exist together, as one.

Just because they sometimes come from somewhere else doesn't mean they are distinct, they are still a direct dependency because the associated source is too.

Typescript isn't a dependency because our package doesn't import anything from it. It is a tool used at build time.

In definitely typed, for example, we must always define types we depend on. Otherwise the consumer can't consume our types out of the box. The same applies here

43081j avatar Jul 18 '19 17:07 43081j

I can see your point of view but a language feature isn't the same as something external tooling parses (jsdoc).

In the eyes of the ECMAScript standard and Node.js, there is no difference between JSDoc language and TypeScript language features--just that one is neatly hidden in comments and one is injected around the code.

Currently, TypeScript is as much an external tool as a JSDoc parser or a JavaScript parser (babel) or a build framework (webpack, gulp, ...) or a test framework... etc. Nothing "runs" TypeScript1. Instead, an equivalent effect is achieved by running JavaScript produced by "building" TypeScript source code. That means that the entire superset of language features offered by TypeScript are equally nil as JSDoc language features when a JavaScript parser comes across them.

Imagine for a moment that all types for packages existed alongside their package. This is the way it should be, the types repo is a workaround for the fact not all packages ship types.

I would love to live in a world like that, but it's not the one we live in, and I expect that as long as JavaScript and TypeScript are separate and both popular that will never be the world we live in. There will always be JavaScript packages authored by developers who don't know about TypeScript / don't know how to write types for their package / don't find typings important enough to include in their package. Always2. DefinitelyTyped will not cease being necessary until it becomes obsolete by the TypeScript compiler's ability to analyze JavaScript code or the mainstream rise of executing TypeScript--whichever comes first.

This fact that TypeScript is not the language of execution means that code that is executed (JavaScript) and the typings that describe that code (declaration .d.ts files) are fundamentally separate. They should ideally always be found in pairs, but that's not a concrete guarantee, and very often not the case.

That means they are separate parts. Each part can be used separately, and some people just like to use them together. That also means that dependency semantics apply to them separately.


In definitely typed, for example, we must always define types we depend on. Otherwise the consumer can't consume our types out of the box. The same applies here

I think using DT as an example furthers the "in favor" side. @types/* packages, or DefinitelyTyped packages, must include the types that are used in their declaration files (.d.ts) files because those are the types read by package consumers. Those DT packages don't care about including @types/* of the package they mock if those types aren't used in the declaration file.

A contrived example

For example, say there's a package xyz. Package xyz has dependencies foo and bar:

xyz
- foo
- bar

Package xyz's source code looks something like:

// xyz/index.js
import Foo from 'foo';
import bar from 'bar';

export function xyz() {
  bar.baz();
  return new Foo();
}

The @types/xyz package would look like:

// @types/xyz/index.d.ts
import Foo from '@types/foo';

export function xyz(): Foo;

And the @types/xyz package would have just one dependency:

@types/xyz
- @types/foo

In this kind of situation, yes, you do need to include all @types/* you use because the .d.ts typings you write are directly consumed when others use your DefinitelyTyped package. If someone installs @types/xyz as a dependency, their TypeScript compiler will read and interpret the index.d.ts that you wrote.

So what's the difference?

We do not write our typings in this repo. They are generated by the TypeScript compiler. The TypeScript source we write is not equal to the declaration (.d.ts) files generated. When someone installs @slack/web-api and their local TypeScript compiler goes to node_modules/@slack/web-api/ and starts reading typings files, it does not read the source code we author located in src/--in fact, the src/ directory isn't published to npm nor is it in anyone's node_modules/ directory because those .ts files are directly irrelevant to package consumers.

Those .ts files currently have one purpose: to generate the distributable files (and human readability/convenience, I suppose).

[X] isn't a dependency because our package doesn't import anything from it.

Apply this logic to the distributed .js and .d.ts files. @types/debug is never imported by a given project's executed .js files or interpreted .d.ts files. Never. It has no reason to be a "dependency".

It's used in the build process, though. Just like typescript. And typescript is a dev dependency because it's never imported by the code that package consumers download and run/their tools interpret. And some (some) @types/* packages are used during build, but not after. So they, too, should be dev dependencies.


Edit:

Just to be entirely clear, I'm personally aligned with the idea that all packages should have types and that the two should be a pair. But I'm afraid that's just a dream for now 😕. This issue aims to make the best of what we have.

If an example of what kind of effect this change would have would be helpful, I'm happy to set up a demonstration and bring for show-and-tell 😄

Notes

[1] This is the point of divergence. TypeScript code doesn't run. Even if something did run TypeScript in its natural form, this divide would still exist so long as TypeScript needs to compile to JavaScript, and that would likely be the case for as long as JavaScript is a major programming language--a fact I don't expect will change very soon.

[2] Okay. Maybe not "always", but for the foreseeable future. Long enough that a small change like this can scale massively over time. And there's also the possibility that it may indeed be always.

clavin avatar Jul 19 '19 16:07 clavin

i mean this still drills down to the fact that we want the package to be consumable by typescript users.

in order to do that, you should (as DT does) declare the dependencies.

if one of the target runtimes for this repo wasn't typescript, sure, you wouldn't care about the declarations being useable out of the box and you wouldn't declare any of the dependencies. but it is a target, so we must do what every other repository does in this situation (as far as ive seen over the years) and ship the types as dependencies.

you import lodash? declare @types/lodash as a dependency, otherwise the types produced will not work out of the box and shouldn't. because you forgot a dependency as far as the TS compiler is concerned (which is a target runtime).

use anything from node's standard library? its again a dependency (@types/node) and the shipped types will not work without it.

i think what you're possibly missing here is that, no, JSDoc and TypeScript are not the same. TypeScript is a target runtime this package supports, just like if we started shipping for Deno or any other runtime. It doesn't matter if these features are not part of the ES specs, they are part of a runtime we support. JSDoc is some external tool, it is not a target runtime.

this usually isn't a difficult problem or discussion because almost all typescript projects generally declare dependencies this way: third-party types and sources consumed by the package's shipped code are declared in dependencies.

43081j avatar Jul 22 '19 17:07 43081j

Just wanted to check in here since its been a little while.

I'm not certain the algorithm @clavin has described is a perfect test for when we're over or under exposed @types/* packages via the "dependencies" of a package - but I am happy to try and see what we find out! I think the next step here would be for a contributor to offer a checker that implements this algorithm, and we could set it up to run on all PRs for a trial period of time. If we see it do the right thing that we as humans would do, and we begin to trust this check, then we could turn it into a required check.

I want to acknowledge that this contribution is a pretty low priority for the project right now. There's just not a whole lot to be gained (as measured by the "In favor" and "Against" lists in the initial comment). There are much more meaningful tasks that someone could pick up. But if this is the most interesting idea in the entire issue tracker to someone - then I'm not going to discourage them at all!

aoberoi avatar Nov 19 '19 07:11 aoberoi

👋 It looks like this issue has been open for 30 days with no activity. We'll mark this as stale for now, and wait 10 days for an update or for further comment before closing this issue out. If you think this issue needs to be prioritized, please comment to get the thread going again! Maintainers also review issues marked as stale on a regular basis and comment or adjust status if the issue needs to be reprioritized.

github-actions[bot] avatar Oct 30 '23 00:10 github-actions[bot]

As this issue has been inactive for more than one month, we will be closing it. Thank you to all the participants! If you would like to raise a related issue, please create a new issue which includes your specific details and references this issue number.

github-actions[bot] avatar Nov 13 '23 00:11 github-actions[bot]