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

chore(parse-duration): convert to TypeScript #7981

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

velvetrevolver
Copy link
Contributor

@velvetrevolver velvetrevolver commented Sep 10, 2024

Description

Convert parseDuration to TS

  • changes in eslintrc.json to run tsc in @vates directory
  • index.ts created in /src
  • parseDuration index.js moved from / to /dist
  • change in parseDuration package.json : set main: /dist/index.js

XO-162

Checklist

  • Commit
    • Title follows commit conventions
    • Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
    • If bug fix, add Introduced by
  • Changelog
    • If visible by XOA users, add changelog entry
    • Update "Packages to release" in CHANGELOG.unreleased.md
  • PR
    • If UI changes, add screenshots
    • If not finished or not tested, open as Draft

.eslintrc.js Outdated Show resolved Hide resolved
@julien-f julien-f changed the title chore: convert parseDuration to TS chore(parse-duration): convert to TypeScript Sep 12, 2024
@velvetrevolver velvetrevolver self-assigned this Sep 13, 2024
@julien-f julien-f self-requested a review September 19, 2024 09:35
@julien-f julien-f removed the request for review from fbeauchamp September 22, 2024 18:51
Copy link
Member

@julien-f julien-f left a comment

Choose a reason for hiding this comment

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

Rebase on top of master, the code has changed because I merged your other PR.

@vates/parse-duration/package.json Outdated Show resolved Hide resolved
@@ -2,11 +2,11 @@

const ms = require('ms')

exports.parseDuration = value => {
exports.parseDuration = (value: number | string): number => {
Copy link
Member

Choose a reason for hiding this comment

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

parseDuration will throw for other types, should we accept them in the types or not?

I don't know the best practice for 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.

We should only accept types that can be used by the function (and avoid "any").
If at runtime we should have data of an unintended type, this is handled by error handling.

@vates/parse-duration/src/index.ts Outdated Show resolved Hide resolved
"rootDir": "./src"
},
"exclude": ["node_modules", "dist"]
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this config, too many settings, and too many I don't understand.

What do you think of delegating the tedious configuration to the community?

Something like: https://github.com/julien-f/untitled/blob/dba7b3404da054f906a0f017daede66f7378e281/tsconfig.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to keep control over this file. More than just a configuration, it is what allows us to define the quality of code we want.

@velvetrevolver velvetrevolver force-pushed the convert-ts-parse-duration branch 2 times, most recently from 3f08791 to 9301b10 Compare September 24, 2024 08:04
@velvetrevolver
Copy link
Contributor Author

CI failed because of eslint. I had to put TS options in it back.

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.

4 participants