Bring back the uPlot typescript namespace

[ghost]

Hey @leeoniya :wave: Since this commit from v1.4.0, all the types/interfaces related to uPlot are no longer in a namespace:

Before

import uPlot from 'uplot'

const axisStyles: uPlot.Axis = {
    font: `500 12px ${htmlStyles.fontFamily}`,
    stroke: htmlStyles.color,
    grid: { stroke: 'rgba(0,0,0, 0.05)' },
    ticks: { show: false }
}

const plot = new uPlot()

After

 // Loss of information about where the Axis type comes from.
import uPlot from 'uplot'
import type { Axis } from 'uplot'

const axisStyles: Axis = {
}

const plot = new uPlot()

or

// Having to import uplot twice.
import uPlot from 'uplot'
import type * as uPlotTypes from 'uplot'

const axisStyles: uPlotTypes.Axis = {
}

const plot = new uPlot()

or

// This is the closest to where we had before, but having to access the
// `default` export for the constructor is not good.
import * as uPlot from 'uplot'

const axisStyles: uPlot.Axis = {
}

const plot = new uPlot.default()

With a namespace that is named the same as the uPlot class like it was before, we can use the TypeScript and JavaScript values at the same time.

[leeoniya]

why not

import uPlot, * as uType from "uplot";

const axis: uType.Axis = {...};

const plot = new uPlot();

[ghost]

I would still have two differently named references to the same package.

[leeoniya]

can you check if this still plays nice with module augmentation: https://github.com/leeoniya/uPlot/issues/441#issuecomment-771247728

[ghost]

I tested augmenting a class with a namespace here. Is this what you wanted to check?

[leeoniya]

yeah, that looks like it works. is there any difference between declare namespace and declare module? they seem to act the same way. the docs say that modules cannot be concatenated, but it looks like they just get merged if you repeat them in the same file.

any drawbacks to just using module instead of namespace?

using namespaces in both places works too: https://www.typescriptlang.org/play?#code/CYUwxgNghgTiAEkoGdnwK4AUIHsAu8A3gFAC+xxoSc8AdlALYjIAOUYCWuBJ8f8AS1p4QMAGbsEAZVEDmRYvyXwoALngAjHDgggotRf3LlK4aDXpNWkjNnwKlQkeJsyYctL2V8N6rTr0DJWMKMBxaZAJkWWZ1LnwAOjcPeABeIhV1PBh0EAAaTSychFJ4AHoy+ABhfRUwDlR4PABPFnl9YBV0AHMmYSaACxAGUPDI+BZuNLoQAHdbbgAKAEpyypraRDHs9DACPCHEaFRiIA

i guess namespace is specifically a types-only thing and should not include actual code, so that's probably the way to go.

https://www.typescriptlang.org/docs/handbook/namespaces-and-modules.html

[ghost]

quoting https://stackoverflow.com/questions/37119215/what-is-the-difference-between-namespaces-and-modules-in-typescript

As it is stated in the TS-handbook there are 2 kind of modules: "internal" & "external". The code in the internal module is written in Typescript and the "external" is written in Javascript.

In order to align with new ECMAScript 2015's terminology they decided to rename them as follows:

"Internal modules" are now "namespaces". "External modules" are now simply "modules", as to align with ECMAScript

Yeah seems like you guessed right.

[leeoniya]

want to PR this and i'll test how it drops into Grafana?

[ghost]

I'm almost done with the commit here, but while I was going through the declaration file I found a problem unrelated to this issue I opened, just a observation: if you import the const enums that are in the declaration file it would seem ok in TS but would be undefined at runtime. This is because declaration files do not emit any javascript, so in order for the enums to work they need to exist in the javascript source code.

Basically, if you do this:

import uPlot, { DrawOrderKey } from 'uplot'
console.log(DrawOrderKey.Axes)

TS assumes that the DrawOrderKey object exists in the JS file, but at runtime it would be undefined or your bundler would crash.

This has nothing to do with the namespace issue by the way

[leeoniya]

i'm fairly certain this is an old babel-specific issue. see https://github.com/babel/babel/issues/8741, https://github.com/grafana/grafana/issues/29293.

const enums are specifically there compile down to their values without overhead, and the real TS compiler does in fact do this.

https://www.typescriptlang.org/docs/handbook/enums.html#const-enums

demo:

https://www.typescriptlang.org/play?#code/MYewdgzgLgBApmArgWxgEQJYCc7Ch8GAbwCgYYBVABwBoz0QB3MO8gGTgDMpWYAlDAHMAFjxIBfEiQA2cWABNsufOAgwAvDADa9TDjwEwAOmq89yw0bRMWupQfBGO3M-ZXGBIsQF0A3EA

this looks like the "solution": https://github.com/dosentmatter/babel-plugin-const-enum

[ghost]

But does TS compiles enums that are only in declaration files? From what I understand, you can't import "real" values from .d.ts files, so the uPlot source code would have to be written in TS I think. Also I'm using esbuild instead of babel, and when I tried to import some enum like DrawOrderKey it looked for the exported enum in the JS source code and crashed, I guess it would be the same for webpack and rollup.

[leeoniya]

But does TS compiles enums that are only in declaration files?

that's a good question, and worth testing with a raw TS compiler.

From what I understand, you can't import "real" values from .d.ts files

right, but const enums are not "real" values. i would expect that the compiler would behave the same way for type-only transpilations regardless of where they come from, why shouldn't it?

[leeoniya]

i tested it with these files in the same directory:

uPlot.d.ts:

export const enum Foo {
    Bar = 1,
    Moo = 2,
}

test.ts:

import { Foo } from './uPlot';

console.log(Foo.Bar);

CLI (TypeScript 4.1.3):

tsc test.ts

output (test.js):

"use strict";
exports.__esModule = true;
console.log(1 /* Bar */);

[ghost]

That's good to know :thinking: I tried with esbuild but turns out it differs from tsc:

$ esbuild --bundle test.ts
 > test.ts: error: Could not resolve "./uPlot"
    1 │ import { Foo } from './uPlot';
      ╵                     ~~~~~~~~~

[ghost]

Using namespace also works:

uPlot.d.ts:

declare namespace uPlot {
    export const enum Foo {
        Bar = 1,
        Moo = 2,
    }
}

export default uPlot

test.ts:

import uPlot from './uPlot';

console.log(uPlot.Foo.Bar);

test.js:

"use strict";
exports.__esModule = true;
console.log(1 /* Bar */);

[leeoniya]

this doesnt seem like a difficult thing to support, and is fairly straightforward without tricky corner cases. i think if you open an esbuild issue, evan would add it.

[leeoniya]

well, we have a lot of code like this in Grafana currently:

which would need to be reworked.

we did have some internal discussion during the namespace removal and the general thought was that enforcing the uPlot prefix to every type was maybe not too necessary since more often than not the types are unique and you can always alias via import {Axis as uAxis} if you need the extra disambiguation, and generally you wouldn't.

i'm not 100% convinced that having to import uPlot twice and using uPlotTypes.Axis is that big a deal. i agree that using uPlot.default() is not good, but the other import styles don't require this (and technically you're still using the default export, just without the keyword, right?).

i feel the whole thing is rather subjective and dependent on the coding style. i guess there is a compelling argument that React uses a namespace for all its types.

cc @zefirka thoughts?

one thing is for sure: this won't happen in a 1.6.x release.

[ghost]

I don't know why that import statement in your screenshot doesn't work when using a namespace, since in react you can do:

import React from 'react'
type T = React.ReactNode

or

import React, { ReactNode } from 'react'
type T = ReactNode

[zefirka]

I don't have a strong opinion due to this topic is mostly about code style. Currently we're using

import uPlot, {Scale as uScale, Options as uOptions} from 'uplot';

So I prefer to keep it this way. But If this will work I don't see why not to allow to use both styles.

[leeoniya]

yeah, ideally we can get it working both ways.

[ghost]

I took a look at how the react declarations are defined and I got this working:

uPlot.d.ts:

declare class uPlot {
}

export = uPlot;

declare namespace uPlot {
    interface Series {
        k: boolean
    }
}

export as namespace uPlot;

test.ts:

import uPlot, { Series } from './uPlot'

const s1: Series = { k: true }

const s2: uPlot.Series = { k: true }

const p = new uPlot()

[leeoniya]

Grafana built without errors, so that's looking better :)

but it looks like this method screws up module augmentation? it complains about "Import declaration conflicts with local declaration of uPlot".

so rather than extending it, it masks it.

import uPlot, { Series } from './uPlot'

declare namespace uPlot {
    interface Series {
        k: boolean
    }
}

const s1: Series = { k: true };

const s2: uPlot.Series = { k: true };

const p = new uPlot();

[zefirka]

If it screws module augmentation, I would rather prefer module augmentation not namespaces. Because namespaces are just about how it looks, but module augmentation allows you to extend types through whole module which, I guess, really useful feature when you're about to implement wrappers and plugins for uPlot.

[leeoniya]

i don't think module augmentation works with the way React types are done, either:

[leeoniya]

ok, i tried this again and it does all work properly if you use declare module 'uplot':

import uPlot, { Series, Options } from 'uplot';

declare module 'uplot' {
  interface Series {
    foo?: boolean
  }
}

const s1: Series       = { foo: true, stroke: "red" };
const s2: uPlot.Series = { foo: true, stroke: "red" };

let opts: Options = {
  width: 300,
  height: 300,
  series: [
    {},
    s1,
    s2,
  ]
};

new uPlot(opts, [[0, 1], [0, 1]], document.body);