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

Ensuring compatibility with new changes. #50

Merged
merged 3 commits into from
Nov 2, 2023
Merged

Conversation

Ashraf-Ali-aa
Copy link
Contributor

  • Update config to support mobile integrate

@@ -1,3 +1,4 @@
/* eslint-disable no-case-declarations */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, it was used from the mono repo code, I'll remove it for now but I might need to add it again.

@@ -16,10 +16,10 @@ describe('calculatePrayerTimes', () => {
},
timezone: 'Europe/London',
calculationMethod: 'MoonsightingCommittee',
asrCalculation: 'hanafi',
asrCalculation: 'Hanafi',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think these naming won't work as our api has different namings

Copy link
Contributor Author

@Ashraf-Ali-aa Ashraf-Ali-aa Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Custom: {
calculationKey: 'Custom',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we called it just name in legacy implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the old code should not using this implementation but I think its method based off https://github.com/prayersconnect/configs/blob/main/src/calc-methods/index.ts#L5

) {
prayerTimeOptions.calculationParameters.madhab = asrCalcSetting;
const type =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -38,6 +44,22 @@ export const getCalcMethodsByCountry = (
};
};

export const getCalculationMethodByCountry = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thehungrycoder The reason why I created another implementation is that it will break older implementations of the code i.e. (web) if they used the updated config, the object structure is slightly different and uses different names for the calculation methods. With this approach, it will be safe to keep upgrading and not affect legacy implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this concerns me as it will cause confusion of a module having two different methods of seemingly same purpose.

unless you can think of a cleaner approach, consider overloading

https://learntypescript.dev/05/l3-overloading

Copy link
Contributor Author

@Ashraf-Ali-aa Ashraf-Ali-aa Oct 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree method overloading would be better, but since the method would be deprecated due to the calculation method names will no longer work in the future, I can add a @deprecated message

/**
 * @deprecated The method should not be used
 */

@@ -112,6 +118,10 @@ export function getConfigByISOName(name: string): ICountryConfig {
...defaultConfig.prayerSettings,
...countryConf?.prayerSettings,
},
calculationSettings: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is same info available under prayerSettings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, I was initially planning on using prayerSettings but I realised it would break legacy implementation so I have separated it out.

@@ -23,6 +24,10 @@ export const defaultConfig = {
asr_method: 'Standard',
calculation_method: 'MWL',
} as IPrayerSettings,
calculationSettings: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you look prayerSettings prop, these two values are available there.

@@ -44,6 +44,7 @@
},
"dependencies": {
"adhan-extended": "^6.1.0",
"luxon": "^3.3.0"
"luxon": "^3.3.0",
"@photostructure/tz-lookup": "^8.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how good is this library? in mobile we use another library. please check and compare both and let's use the better one.

Copy link
Contributor Author

@Ashraf-Ali-aa Ashraf-Ali-aa Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an up-to-date fork of tz-lookup (used in the mobile app) since it is no longer maintained by darkskyapp
https://github.com/photostructure/tz-lookup

@Ashraf-Ali-aa Ashraf-Ali-aa merged commit adf46fa into main Nov 2, 2023
10 checks passed
@Ashraf-Ali-aa Ashraf-Ali-aa deleted the update-config branch November 2, 2023 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants