batoulapps / adhan-js

High precision Islamic prayer time library for JavaScript
MIT License
385 stars 86 forks source link

CalculationParameters type improvment #38

Closed khawarizmus closed 3 years ago

khawarizmus commented 3 years ago

Assalam alykoum

Shouldn't the following constructor signature in the type definitions that is found here use optional arguments?

Basically changing this:

constructor(methodName: string|undefined|null, fajrAngle: number, ishaAngle: number, ishaInterval: number, maghribAngle: number)

To this:

constructor(methodName?: string, fajrAngle: number, ishaAngle: number, ishaInterval?: number, maghribAngle?: number)

Currently, all arguments are required except for methodName that is implied from undefined and null type that was added. but looking at the code only fajrAngle and ishaAngle seem required to me.

Also for library authors who use this library as a base (for example allow users to use a custom method). it would be useful to expose all params.

Currently methodAdjustments, maghribAngle, and polarCircleResolution are missing from the type definitions making the library throw an error when for example trying to assign custom method adjustments or any one of the three params mentioned above.

public initWithCustomMethod(config: PrayersCustomConfig) {
    this.config = config
    const { date, latitude, longitude, dateFormatter, ...paramsOptions } = this.config as PrayersCustomConfig;
    const calculationParams = new CalculationParameters(paramsOptions.fajrAngle || 18, paramsOptions.ishaAngle || 18, paramsOptions.ishaInterval || 0, 'Other');
    calculationParams.methodAdjustments = paramsOptions.methodAdjustments // this throws an error
    calculationParams.adjustments = paramsOptions.adjustments;
// ...
  }

This small change would bring better type checking and intelisence as well as prevent few type errors. and I can help PR the changes.

z3bi commented 3 years ago

This is a great suggestion. This is definitely an oversight on our part. The only thing that shouldn't be exposed is methodAdjustments as that is meant to be an internal value used by the pre-defined methods. Custom methods would simply use the adjustments param.

It seems the TypeScript definition is also missing the qibla definition (as you noted in your other issue) and also the tehran calculation method. Perhaps we can bundle all these changes into one PR to update the typescript definition.

khawarizmus commented 3 years ago

Pr added #39