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

Convert to Typescript? #141

Open
mimshwright opened this issue May 31, 2022 · 14 comments
Open

Convert to Typescript? #141

mimshwright opened this issue May 31, 2022 · 14 comments
Assignees

Comments

@mimshwright
Copy link
Contributor

Mentioned by @DJTB in #139

Hey, what if we converted to ts? Would there be interest?

I took a quick look at the effort involved. It doesn't look like converting the code would take long at all and I'd be happy to do it. However, hacking together the right config files to make it compile might and I'd rather not do that. Perhaps it might even be more prudent to try copying the source to a new ts-dx or vite boilerplate.

@DJTB
Copy link
Collaborator

DJTB commented Jun 1, 2022

All for typescript, considered it myself once or twice for this repo but didn't find the time yet.

I haven't used either of the boilerplates referenced, but ts-dx looks to have almost the same setup - I'm happy with everything that it comes included with. I'm not precious at all for the current eslint config - it was there since the project was JS only - happy to go with new defaults. TS pretty much covers our bases on that front anyway.

I'd say go with that and let's get existing functionality + jest tests working - then I could take a look at re-adding cypress tests pre-release.

@mimshwright mimshwright self-assigned this Jun 4, 2022
@mimshwright
Copy link
Contributor Author

I'm giving it a shot.

@mimshwright
Copy link
Contributor Author

mimshwright commented Jun 4, 2022

@DJTB Do you know anything about tokenize( input, {detailed: true})? It doesn't appear to be used internally at all except in a few tests. Maybe we can make it a separate function to simplify the type checks.

@DJTB
Copy link
Collaborator

DJTB commented Jun 4, 2022 via email

@mimshwright
Copy link
Contributor Author

Turns out tsdx is what I started with and it's no longer being maintained. Going to see if I can find some other alternative.
Furthermore, I had been working from my own fork without realizing it so i started doing the conversion on code that was several months old. Now i'm kinda stuck in a corner. Blegh.
I think what may be best is if i can try to move the project to a very barebones setup with nothing but ts, lint, jest. Then I can do the ts conversion and push it up for you to have a look at.

@mimshwright
Copy link
Contributor Author

If I'm being honest (with myself) i kind of have a ton of stuff to do right now and i think i took this on as a form of procrastination. I might need to step away for a while and not touch it until i actually have the free time.

@DJTB
Copy link
Collaborator

DJTB commented Jun 5, 2022

No problem, I know the feeling!
If I have some downtime I might try adding TS to the current setup to at least enable a conversion to occur easily.

@mimshwright
Copy link
Contributor Author

I did do a fair amount of rework to typescript so if you start this process, let me know and I'll send you what I have. the main issue was just as i feared, wrestling with the config files. JS builds are really too complicated.

@Naoto-Ida
Copy link

Hey, I've been wanting to help out with a potential TS conversion of react-furi, but then realized that it probably makes sense to convert this project first.

I've had quite some experience now converting to TS without ts-dx, but do agree that Vite might be a nice way to go, since this project already has Rollup :)

I'm gonna give it a go and let y'all know if there's progress.

@DJTB
Copy link
Collaborator

DJTB commented Dec 1, 2022

Awesome, any contributions are more than welcome!
I think react-furi would be a fairly quick conversion afterwards.

@mimshwright
Copy link
Contributor Author

@Naoto-Ida @DJTB
As discussed above, TS-DX seemed to be dead so for what its' worth I've been using this template for npm projects using Vite and it's been working great for me. https://github.com/mimshwright/vite-library-template

TabithaLarkin added a commit to TabithaLarkin/WanaKana that referenced this issue Oct 21, 2023
…compiler for type definitions

As part of adding Typescript, a number of dependencies had to be updated. All @babel packages and
@rollup packages were updated. Additionally prettier-eslint-cli had to be updated due to a typing
issue in the dependency tree. This also meant that eslint also had to be updated to maintain
compatibility. Due to these package changes the minumum node devEngines version has been bumped to
16. A tsconfig.json file has been added, as well as renaming the base index file from index.js to
index.ts.

Adds packages to enable WaniKani#141
TabithaLarkin added a commit to TabithaLarkin/WanaKana that referenced this issue Oct 21, 2023
…e definition output

constants.js has been changed to a typescript file to enable the exporting of the DefaultOptions
type. This is then used by various function definitions, which provides type checking for the
parameters. Regexp has been changed to RegExp as Regexp was resolving to the any type. Other
parameters defined as Object have been specified with types to match their expected input.
TOKEN_TYPES's export was removed as it was unused and was being generated into the d.ts file.

Resolve external typings for WaniKani#141
TabithaLarkin added a commit to TabithaLarkin/WanaKana that referenced this issue Nov 8, 2023
…les and typings

Added better-docs to piggyback off of its typescript plugin. Updated instances of string to String
and boolean to Boolean. This matches the rest of the existing documentation. Typescript types are
unaffected by these changes and automatically get converted to the correct typescript type. Changed
instances of [index: string]: string to Object.<string, string> JSDoc type. This gets converted in
typescript, but the previous syntax was not supported in JSDocs.

WaniKani#141
@TabithaLarkin
Copy link
Contributor

Hey all! I just wanted to let people know that typescript is now available in the repo. In the end I opted for a smaller change than moving to a new build system or organisation of the library, this way we can more incrementally move things over when we're able to.

I see that @mimshwright and @Naoto-Ida have mentioned making some changes for typescript. It should be possible to move stuff over if you think it'll be useful!

@flazouh
Copy link

flazouh commented Mar 31, 2024

Hi, is this normal ?

const tokens = tokenize(input, { detailed: true }) as Array<{
    type: string
    value: string
  }>
  TS2345: Argument of type { detailed: true; } is not assignable to parameter of type
  
{   compact: boolean | undefined;   detailed: boolean | undefined; }

Property compact is missing in type { detailed: true; } but required in type

{   compact: boolean | undefined;   detailed: boolean | undefined; }

Shouldn't you be able to put only one of them since they do 2 differents things based on

If { compact: true } then many same-language tokens are combined (spaces + text, kanji + kana, numeral + punctuation). If { detailed: true } then return array will contain { type, value } instead of 'value'

@DJTB
Copy link
Collaborator

DJTB commented Apr 3, 2024

Hi, is this normal ?

const tokens = tokenize(input, { detailed: true }) as Array<{
    type: string
    value: string
  }>
  TS2345: Argument of type { detailed: true; } is not assignable to parameter of type
  
{   compact: boolean | undefined;   detailed: boolean | undefined; }

Property compact is missing in type { detailed: true; } but required in type

{   compact: boolean | undefined;   detailed: boolean | undefined; }

Shouldn't you be able to put only one of them since they do 2 differents things based on

If { compact: true } then many same-language tokens are combined (spaces + text, kanji + kana, numeral + punctuation). If { detailed: true } then return array will contain { type, value } instead of 'value'

This appears to have been an oversight when type annotations were added.
Should be fixed when #172 is merged and released.

Note this is only a type error, the function can happily accept an object with only one of the keys.
So feel free to temporarily ts-ignore or pass the other key with undefined { compact: undefined, detailed: true }.

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

No branches or pull requests

5 participants