timotejroiko / sweph

The definitive Swiss Ephemeris bindings for Node.js
Other
93 stars 15 forks source link

Problem when using constants as array indices in TypeScript #3

Closed krisztianb closed 2 years ago

krisztianb commented 2 years ago

Hello again. I started using your library and it looks nice, so let me first thank you for all the work you've put into it and making it available to us all.

While using the library in a TypeScript project I continually run into the following problem: Planets are defined via an enum (e.g. sweph.constants.SE_MARS), but whenever I try to use them as numbers for an array index I get a TSC error.

Element implicitly has an 'any' type because index expression is not of type 'number'. ts(7015)

Example:

const array = new Array<number>();
[
    sweph.constants.SE_SUN,
    sweph.constants.SE_MOON,
    sweph.constants.SE_MERCURY,
    sweph.constants.SE_VENUS,
    sweph.constants.SE_MARS,
    sweph.constants.SE_JUPITER,
    sweph.constants.SE_SATURN,
    sweph.constants.SE_URANUS,
    sweph.constants.SE_NEPTUNE,
    sweph.constants.SE_PLUTO,
].forEach((planetId) => {
    array[planetId] = 123456789; // <-- ERROR
});

I think the problem is that not all enum members are numbers. Would it be possible to type the constants as object properties instead of using an enum?

This should only affect the type definitions and not the rest of the library.

Ahriana commented 2 years ago

you are trying to assign numbers to enums which is why your getting the errors if you type the array correctly can can explicitly use numbers no issue but you should prob case to enum first for sanity checking

const myArray: ConstantsType[] = [
    // values
]

myArray[key] = value
timotejroiko commented 2 years ago

hey krisztian, i just tested your code and did not get any error here, using typescript 4.4 with default options. I also tried to intentionally add a string constant to the list and it still worked, so im not sure whats going on on your end :/

timotejroiko commented 2 years ago

after some further research, it seems your issue is caused by "noImplicitAny": true but can be circumvented by setting "suppressImplicitAnyIndexErrors": true.

another way is to define the type of your forEach parameter: planetId: number

timotejroiko commented 2 years ago

also to clarify, the reason i went with const enums was to enable intellisense to show the actual value of the constant on hover instead of just "number" or "string", and from what i gathered, a const enum was the best way to do it at the time. It also makes the code slightly faster as all constants are inlined into the compiled code.

but perhaps you are right, i noticed there is better type resolution with const objects, i'll look into it

krisztianb commented 2 years ago

@Ahriana I think you don't understand the problem yet. I don't assign numbers to enums. I'm trying to use enums as indexes.

@timotejroiko defining the constants type like this works:

export const constants: {
    OK : 0,
    ERR : -1,
    SE_AUNIT_TO_KM : 149597870.7,
    SE_AUNIT_TO_LIGHTYEAR : 0.000015812507409819728,
    SE_AUNIT_TO_PARSEC : 0.000004848136811095274,
    SE_JUL_CAL : 0,
    SE_GREG_CAL : 1,
    SE_ECL_NUT : -1,
    // and so on ...
};

This still gives you IntelliSense and you have better type resolution. I can submit a PR if you want.

timotejroiko commented 2 years ago

@krisztianb

Hey, sorry for the delay, i pushed it to master, could you test it? npm i timotejroiko/sweph

i also added a proper default export, let me know if it works properly.

krisztianb commented 2 years ago

Thanks @timotejroiko no need to apologize. If it's one thing open source software has thought me then it's patience.

I'll test it soon and let you know.

krisztianb commented 2 years ago

Tested it. It works. 👍

timotejroiko commented 2 years ago

@krisztianb released

krisztianb commented 2 years ago

Great job. Thank you very much.