Switch to typescript

[abumalick]

Would you accept a PR to switch to typescript?

I can make it more readable inchallah. I was a bit aggressive on this one:

• I formatted most files using Prettier
• I switched to pika to build the package
• I removed the build file from the repository

I can remove some of these and make it more readable (commit after renaming js files to ts so that you can check actual changes).

I wanted to ask you before working on this actually.

[abumalick]

@z3bi السلام عليكم What do you think about this?

[z3bi]

@abumalick this is awesome! I'm 100% onboard with turning this into typescript.

I'm not familiar with pika, what are the advantages of that compared to other package managers?

I think it should be ok to remove the built file from the repo, but we would want to update the ci pipeline to generate an artifact with releases. We will probably also want to keep the import the same if possible to avoid breaking changes.

[abumalick]

Happy that you are onboard!

You can read on pika here for example: https://www.pika.dev/blog/introducing-pika-pack Basically it simplifies the configuration for creating packages. Also @FredKSchott that is maintaining is working a lot on having packages supporting new browser features, so they will keep optimizing it without needing from us to dig into the building process. We can just use it and not think too much about it. It is using babel and rollup but is simplifying the configuration for packages maintainers.

It is noted for the built file, I will check that in the end inchallah.

I will hard push more organized commits inchallah to show you the actual changes to the files. I am trying to not have any breaking changes.

[abumalick]

@z3bi You can check https://github.com/batoulapps/adhan-js/pull/93/commits/788bd2d35f7d941087f31d8cb47650e9467734a7 to see the exact changes to the files that I pushed to make it work with typescript.

I dropped pika pack in the end, in favor of https://github.com/developit/microbundle (pika didn't have a new release for 2 years). It does basically the same, it handles most of the configuration by itself.

I think it should be ok to remove the built file from the repo, but we would want to update the ci pipeline to generate an artifact with releases. We will probably also want to keep the import the same if possible to avoid breaking changes.

I added the build to the release job. It should work, I didn't test it yet through, the pipeline passes in my github but I didn't increase the version so no release for now. https://github.com/abumalick/adhan-js/actions/runs/2089942331

So the only change to the final package is that the built files are inside the dist folder, we have now multiple build files (commonJs, umd, etc...)

"exports": {
    "require": "./dist/Adhan.cjs",
    "default": "./dist/Adhan.modern.js"
  },
  "main": "./dist/Adhan.cjs",
  "umd:main": "dist/Adhan.umd.js",
  "module": "./dist/Adhan.module.js",
  "unpkg": "./dist/Adhan.umd.js",
  "types": "./dist/Adhan.d.ts",

I believe it should not break anyone's usage of the package, but I would at least bump a minor version as it has important changes and might break in some extreme cases. What do you think? Should I bump to 4.4.0 or 5.0.0?

I will try to test the release assets when you tell me what is the version that I should use inchallah

[abumalick]

@z3bi Do you have time to check my comments please ?

[sgtsquiggs]

Hi @abumalick, I've already got a typescript rewrite in progress. When it is ready I'll post it and we can compare/contrast.

[abumalick]

This PR is not a rewrite, @sgtsquiggs , it is more a transition to typescript. I only added the types that are necessary and didn't change the code structure. There is some refactoring that could be done for a prettier typescript version, like use enums instead of objects.

You can check the typescript adaptation in this commit: https://github.com/batoulapps/adhan-js/pull/93/commits/6f17cae1ddabdbbd5e7b1fbc1f892ed12ab3357c

[abumalick]

@z3bi Any follow up?

[sgtsquiggs]

This would be easier to review if the js -> ts conversion was registered as a diff instead of a delete/create - possibly the eslint/prettier rules caused too many changes for git to automatically associate the two files as related. Could you submit prettier/eslint change as a separate MR that we could merge before this ts MR?

[sgtsquiggs]

Also can you explain the need to rename the config file extensions js -> cjs ? This seems uncommon to me.

[sgtsquiggs]

For packaging - instead of webpack/pika/microbundle I would prefer to use rollup. I was in the process of updating our bundling:

rollup.config.js

import babel from "@rollup/plugin-babel";
import resolve from "@rollup/plugin-node-resolve";
import { terser } from "rollup-plugin-terser";

const extensions = [".js", ".ts"];

export default {
  input: "src/Adhan.js",
  output: [
    {
      file: "lib/bundles/bundle.esm.js",
      format: "esm",
      sourcemap: true,
    },
    {
      file: "lib/bundles/bundle.esm.min.js",
      format: "esm",
      plugins: [terser()],
      sourcemap: true,
    },
    {
      file: "lib/bundles/bundle.umd.js",
      format: "umd",
      name: "myLibrary",
      sourcemap: true,
    },
    {
      file: "lib/bundles/bundle.umd.min.js",
      format: "umd",
      name: "myLibrary",
      plugins: [terser()],
      sourcemap: true,
    },
  ],
  plugins: [
    resolve({ extensions }),
    babel({
      babelHelpers: "bundled",
      exclude: "./node_modules/**",
      extensions,
      include: ["src/**/*.js"],
    }),
  ],
};

With these package.json scripts:

    "clean": "rimraf lib",
    "build:esm": "cross-env BABEL_ENV=esmUnbundled babel src --extensions '.js' --out-dir 'lib/esm' --source-maps",
    "build:cjs": "cross-env BABEL_ENV=cjs babel src --extensions '.js' --out-dir 'lib/cjs' --source-maps",
    "build:bundles": "cross-env BABEL_ENV=esmBundled rollup -c",
    "build": "npm-run-all -l clean -p build:esm build:cjs build:bundles",

[abumalick]

Also can you explain the need to rename the config file extensions js -> cjs ? This seems uncommon to me.

It is because of https://github.com/developit/microbundle requires to enable ECMAScript modules (we need to add "type": "module", to the package.json). After adding this to package.json, running tests will throw this error:

ReferenceError: module is not defined in ES module scope
This file is being treated as an ES module because it has a '.js' file extension and '/path/to/adhan-js/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.

More info: https://nodejs.org/docs/latest-v13.x/api/esm.html#esm_enabling

[abumalick]

This would be easier to review if the js -> ts conversion was registered as a diff instead of a delete/create - possibly the eslint/prettier rules caused too many changes for git to automatically associate the two files as related. Could you submit prettier/eslint change as a separate MR that we could merge before this ts MR?

will do inchallah

[abumalick]

@sgtsquiggs @z3bi Can I drop the Math.trunc polyfill that is in Astronomical file?

[codecov[bot]]

Codecov Report

Merging #93 (a2211f0) into develop (e5c07b4) will increase coverage by 0.40%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop      #93      +/-   ##
===========================================
+ Coverage    98.80%   99.21%   +0.40%     
===========================================
  Files           19       19              
  Lines          587      635      +48     
  Branches       127      120       -7     
===========================================
+ Hits           580      630      +50     
+ Misses           6        5       -1     
+ Partials         1        0       -1     

Impacted Files	Coverage Δ
src/Prayer.ts	100.00% <ø> (ø)
src/Rounding.ts	100.00% <ø> (ø)
src/Shafaq.ts	100.00% <ø> (ø)
src/Adhan.ts	100.00% <100.00%> (ø)
src/Astronomical.ts	100.00% <100.00%> (ø)
src/CalculationMethod.ts	100.00% <100.00%> (ø)
src/CalculationParameters.ts	100.00% <100.00%> (ø)
src/Coordinates.ts	100.00% <100.00%> (ø)
src/DateUtils.ts	100.00% <100.00%> (ø)
src/HighLatitudeRule.ts	100.00% <100.00%> (ø)
... and 21 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c15ba6e...a2211f0. Read the comment docs.

[abumalick]

I believe this PR is ready for review @z3bi @sgtsquiggs 🚀

[sgtsquiggs]

Can you npm remove -D @types/estree @types/jest I believe these are unnecessary

[sgtsquiggs]

@sgtsquiggs @z3bi Can I drop the Math.trunc polyfill that is in Astronomical file?

100%

[abumalick]

I believe @types/jest is necessary to give you typescript support for jest utilities in test files. For @types/estree, I am getting an error when it is not there, I can try again inchallah but it is the only way I found to fix that error.

[sgtsquiggs]

I believe @types/jest is necessary to give you typescript support for jest utilities in test files.

👍

For @types/estree, I am getting an error when it is not there, I can try again inchallah but it is the only way I found to fix that error.

I had ran into the same estree error as you - I was able to get it to stop appearing without adding @types/estree but I forgot what exactly I did. Perhaps we just merge this and fix later.

[abumalick]

Thank you very much @sgtsquiggs 👍🏻

[github-actions[bot]]

:tada: This PR is included in version 4.4.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: