Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Please (re-)export a lot more types and interfaces #19003

Open
yay opened this issue Aug 17, 2023 · 2 comments
Open

[Feature] Please (re-)export a lot more types and interfaces #19003

yay opened this issue Aug 17, 2023 · 2 comments
Labels
en This issue is in English new-feature pending We are not sure about whether this is a bug/new feature.

Comments

@yay
Copy link

yay commented Aug 17, 2023

What problem does this feature solve?

Many eChart's own types are not exported. For example DecalObject.

A lot of the useful types are not exported by eCharts. So one is either faced with a choice to have untyped objects in one's code or to find a way to get to the un-exported types using TypeScript's powerful type system. For example:

import * as echarts from 'echarts';

type EChartInitParams = Parameters<typeof echarts.init>;
// eCharts docs say `It's only optional when opts.ssr is enabled for server-side rendering.` about the `dom` parameter.
// However, the declared type of this parameter is not optional, so we fix it in our derived type.
type EChartInit = (
  dom?: EChartInitParams[0],
  theme?: EChartInitParams[1],
  opts?: EChartInitParams[2]
) => ReturnType<typeof echarts.init>;
// This type is not exported, so we do a few tricks here to get it.
type EChartInitOptions = Exclude<EChartInitParams[2], undefined>;
export type ChartInitOptions = EChartInitOptions & {
  height?: Exclude<EChartInitOptions['height'], string>;
  width?: Exclude<EChartInitOptions['width'], string>;
};

or more generally:

type ExportedOrNotWeCanGetToItAnyway_SoMightAsWellMakeItEasierForUs = echarts.ExportedType['propertyWithUnexportedType'];

Given that we mostly can gain access to un-exported types one way or another through exported ones, the whole point of not exporting types is a bit moot.

zrender types used by eCharts are not re-exported by eCharts

For example, we have to add a transient zrender dependency to our package.json and do something like the following to access to the type that is part of eChart's public interface but is not re-exported:

import * as zrender from 'zrender';

// then somewhere later in our code
highlight?: {
  style?: zrender.PathStyleProps;
  ...
}

What does the proposed API look like?

For eChart's own types:

import * as echarts from 'echarts';

// echarts.ChartInitOptions is exported, so deriving from it becomes a lot easier now
export type ChartInitOptions = echarts.ChartInitOptions & {
  height?: Exclude<echarts.ChartInitOptions['height'], string>;
  width?: Exclude<echarts.ChartInitOptions['width'], string>;
};

For zrender types used by eCharts:

import * as echarts from 'echarts';

// No need to add zrender transient dependency to package.json and import types from there because zrender types used by echarts are re-exported by echarts.
highlight?: {
  style?: echarts.PathStyleProps;
  ...
}
@echarts-bot echarts-bot bot added en This issue is in English pending We are not sure about whether this is a bug/new feature. labels Aug 17, 2023
@yay yay changed the title [Feature] Please export a lot more types and interfaces [Feature] Please (re-)export a lot more types and interfaces Aug 17, 2023
@yay
Copy link
Author

yay commented Sep 11, 2023

There are a few other issues with types:

  1. I would expect types to be declared for event params. For example, I have to declare my own type for legendselectchanged event params passed into the event's callback function:
export type LegendSelectChangedEventParams = {
  // The `name` of the toggled series.
  name: string;
  // A map of series name to selection status (for all series).
  selected: {
    [name: string]: boolean;
  };
  type: 'legendselectchanged';
};

But when using this type like so:

const onLegendSelectChanged = useCallback((params: LegendSelectChangedEventParams) => {
    console.log(params);
  }, []);

useEffect(() => {
    const chart = chartApiRef.current;
    if (!chart) return;

    chart.on('legendselectchanged', onLegendSelectChanged);
    return () => {
      chart.off('legendselectchanged', onLegendSelectChanged);
    };
  }, [chartApiRef, onLegendSelectChanged]);

I would then get a TS error anyway:

Argument of type '(params: LegendSelectChangedEventParams) => void' is not assignable to parameter of type 'WithThisType<(...args: unknown[]) => boolean | void, EChartsType>'.
  Types of parameters 'params' and 'args' are incompatible.
    Type 'unknown' is not assignable to type 'LegendSelectChangedEventParams'.ts(2345)
const onLegendSelectChanged: (params: LegendSelectChangedEventParams) => void

Where the ...args: unknown[] part is coming from here:

declare type ECEventDefinition = {
    [key in ZRElementEventName]: EventCallbackSingleParam<ECElementEvent>;
} & {
    rendered: EventCallbackSingleParam<RenderedEventParam>;
    finished: () => void | boolean;
} & {
    [key: string]: (...args: unknown[]) => void | boolean;
};

  1. There are some typos in type names:
// typo: should be RegionOptions
interface RegoinOption extends GeoStateOption, StatesOptionMixin<GeoStateOption, StatesMixinBase> {
    name?: string;
    selected?: boolean;
    tooltip?: CommonTooltipOption<GeoTooltipFormatterParams>;
}

interface GeoOption extends ComponentOption, BoxLayoutOptionMixin, AnimationOptionMixin, GeoCommonOptionMixin, StatesOptionMixin<GeoStateOption, StatesMixinBase>, GeoStateOption {
    mainType?: 'geo';
    show?: boolean;
    silent?: boolean;
    regions?: RegoinOption[]; // typo: should be RegionOptions
    stateAnimation?: AnimationOptionMixin;
    selectedMode?: 'single' | 'multiple' | boolean;
    selectedMap?: Dictionary<boolean>;
    tooltip?: CommonTooltipOption<GeoTooltipFormatterParams>;
}

@badrylin
Copy link

yes yes yes!!!!!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
en This issue is in English new-feature pending We are not sure about whether this is a bug/new feature.
Projects
None yet
Development

No branches or pull requests

2 participants