Propose new dependency structure for 2.0

[nayeemrmn]

The current Dependency definition incurs an extra layer of depth to support an uncommon case like // @deno-types, we can flatten it with these types:

export type Dependency = {
  key: string;
  specifier?: string; // ┐
  error?: string;     // ┴─ mutually exclusive
  range: Range;
  isTypeOnly?: boolean;
  isDynamic?: boolean;
  externalTypesIndex?: number; // @deno-types is extracted as another dependency, the index pointing to it goes here.
}

export interface EsModule {
  dependencies: Dependency[];
  externalTypes: { // renamed from typesDependency
    key: string; // specifier text
    specifier?: string; // ┐
    error?: string;     // ┴─ mutually exclusive
    range?: Range; // defined for ts reference, not X-TypeScript-Types (currently non-optional field with strange default)
  }
  // ...
}

This module gets the following serializations:

/// <reference types="./external_types.d.ts" />
/// <reference path="./type_only.d.ts" />
import * as a from "./runtime.ts";
// @deno-types="./untyped_runtime.d.ts"
import * as a from "./untyped_runtime.js";
import * as a from "bad.js";

Currently

{
  kind: "esm",
  dependencies: [
    {
      specifier: "./runtime.ts",
      code: {
        specifier: "file:///runtime.ts",
        span: { /*...*/ },
      },
    },
    {
      specifier: "./type_only.d.ts",
      type: {
        specifier: "file:///type_only.d.ts",
        span: { /*...*/ },
      },
    },
    {
      specifier: "./untyped_runtime.js",
      code: {
        specifier: "file:///untyped_runtime.js",
        span: { /*...*/ },
      },
      type: {
        specifier: "file:///untyped_runtime.d.ts",
        span: { /*...*/ },
      },
    },
    {
      specifier: "bad.js",
      code: {
        error: 'Relative import path "bad.js" not prefixed with / or ./ or ../',
        span: { /*...*/ },
      },
    },
  ],
  size: 250,
  typesDependency: {
    specifier: "./external_types.d.ts",
    dependency: {
      specifier: "file:///external_types.d.ts",
      span: { /*...*/ },
    },
  },
  mediaType: "JavaScript",
  specifier: "file:///entry.js",
}

Proposed

{
  kind: "esm",
  dependencies: [
    {
      key: "./runtime.ts",
      specifier: "file:///runtime.ts",
      range: { /*...*/ },
    },
    {
      key: "./type_only.d.ts",
      specifier: "file:///type_only.d.ts",
      range: { /*...*/ },
      isTypeOnly: true,
    },
    {
      key: "./untyped_runtime.js",
      specifier: "file:///untyped_runtime.js",
      range: { /*...*/ },
      externalTypesIndex: 3,
    },
    {
      key: "./untyped_runtime.d.ts",
      specifier: "file:///untyped_runtime.d.ts",
      range: { /*...*/ },
      isTypeOnly: true,
    },
    {
      key: "bad.js",
      error: 'Relative import path "bad.js" not prefixed with / or ./ or ../',
      range: { /*...*/ },
    },
  ],
  size: 250,
  externalTypes: {
    key: "./external_types.d.ts",
    specifier: "file:///external_types.d.ts",
    range: { /*...*/ },
  },
  mediaType: "JavaScript",
  specifier: "file:///entry.js",
}

cc @dsherret

[dsherret]

Although it seems nicer, I'm not sure it's worth changing so drastically at this point because of the amount of work it would require to change the code using the output. I think we should mostly only make changes to the output that do stuff like adding more detail (ex. imports) or fixing the output for bugs.

[nayeemrmn]

There is at least one piece of info that the current structure is missing for seemingly no reason, the relative specifier text associated with a dependency.type from // @deno-types. And there are a lot of smells like why does a ResolutionResolved object contain a range, and why is TypeDependency's range non-optional with a nonsensical default for X-TypeScript-Types? There's too much legacy surrounding Resolution and it's never had a proper overhaul, the non-extensibility of it will keep being an issue.

Although I used the json for illustration, it's really the rust API that's been frustrating me. How open are we to making rust changes and preserving the json in serialize_dependencies?

[dsherret]

Although I used the json for illustration, it's really the rust API that's been frustrating me. How open are we to making rust changes and preserving the json in serialize_dependencies?

Very open. We should discuss major changes before doing them so I appreciate this issue. Also, we should do each change slowly one at a time.

There is at least one piece of info that the current structure is missing for seemingly no reason, the relative specifier text associated with a dependency.type from // @deno-types.

Yeah, I noticed that one, but I'm not sure how important it is to know about for people and the information is somewhat there by using the range against the original text. Is it useful to be able to key on the specifier in this case? Maybe it should just go on the imports like in your other PR?

And there are a lot of smells like why does a ResolutionResolved object contain a range

Agree. I think that should be available regardless of whether it succeeds or not. Also, we should probably remove the specifier property on Range because that's duplicate information that can be figured out by looking at the current module's specifier.

why is TypeDependency's range non-optional with a nonsensical default for X-TypeScript-Types

Agree. I had previously opened #133 about this.

---

What do you think about making these changes one PR at a time and without breaking --json for now? Then in 2.0 we can break it.

Btw, I did a PR in a rush last week to fix a regression and stored the specifier text in the range without thinking. I guess it does give a bit more resolution though... anyway, I am reverting most of it in #249.

[devingfx]

• Please include Range char coordinates (the whole source start char and end chart, not line+char)
• Please include also the whole import statement coordinates (to be able to remove it completely or modify it completely) right now tools need parse out the string before Range.start to find out where the import keyword is located, or what are the local bindings)
• Maybe also include export statement Ranges ?