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

Add types to app-wrapper.js #155

Closed
wants to merge 7 commits into from
Closed

Conversation

lishaduck
Copy link
Contributor

@lishaduck lishaduck commented Jun 15, 2024

I'm unsure as what type flags is supposed to be, beyond that it's not Flags.
I'm pretty sure VSCode was wrong that it's Options.

Relates to: #125

@lishaduck lishaduck changed the title chore: add types to app-wrapper Add types to app-wrapper.js Jun 15, 2024
Comment on lines 5 to 10
/**
* @typedef { import('./types/options').Options } Options
* @typedef { import('./types/options').ReviewOptions } ReviewOptions
* @typedef { import('./types/app').App } App
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once ts 5.5 is stable (which should be soon), we can switch to the import syntax instead here:

Suggested change
/**
* @typedef { import('./types/options').Options } Options
* @typedef { import('./types/options').ReviewOptions } ReviewOptions
* @typedef { import('./types/app').App } App
*/
/**
* @import {Options} from './types/options';
* @import {ReviewOptions} from './types/options';
* @import {App} from './types/app';
*/

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, looking forward to 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.

soon

Today!
I'm currently cleaning up this branch locally, I should have something up today or tomorrow (certainly tomorrow in Europe. How's the future? 😉).

@lishaduck
Copy link
Contributor Author

Oh, I see. This is blocked on #126. I'll try to see if I fix it sometime.

@lishaduck
Copy link
Contributor Author

Ok, the failures from #126 were a bit intimidating, so I figure I'll just port them over here and just coauthor them.

lishaduck and others added 4 commits June 15, 2024 21:06
Co-authored-by: Henrique Buss <henriquebusss@gmail.com>
TS 5.5 might fix this, but we've still got time.
@lishaduck
Copy link
Contributor Author

I plan on relanding this as separate PRs later this week.

@lishaduck lishaduck closed this Jun 16, 2024
@jfmengels
Copy link
Owner

Sounds good, take your time @lishaduck and thank you for looking into this!

@jfmengels
Copy link
Owner

I'm unsure as what type flags is supposed to be, beyond that it's not Flags.
I'm pretty sure VSCode was wrong that it's Options.

Flags is something that looks very much like Options. If I'm not mistaken, it's an object that is passed to the worker_threads that takes a number of fields (with some differences) taken from Options.

@lishaduck
Copy link
Contributor Author

lishaduck commented Jun 21, 2024

Yikes! I'm up to 178 commits on my branch. I really need to split them up 😬

This was referenced Jun 22, 2024
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.

2 participants