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 some code to TypeScript #655

Merged
merged 20 commits into from
Feb 14, 2024
Merged

Conversation

unional
Copy link
Contributor

@unional unional commented Jan 11, 2024

The conversion will be done in many small PRs,
so that the changes can be merged quickly,
minimizing merge conflicts for the team.

The Error component is converted to a functional component, and show that the error prop is not being used.

@alexpargon
Copy link
Contributor

Hey there @unional ! I'm converting everything to Typescript on the newFocusArch and newPreferences branches that contain the 2.0 version of Bazecor, which entails:

  • migrating to Typescript
  • migrating to tailwind and abandoning styled-components & bootstrap completely
  • full rewriting of Focus.API to make it work with USB HID interfaces and better integrate the other functions like virtual keyboards.

so if you want to help out with this tell me and I will incorporate the changes with development to atomize the migration in different PRs so you and others can contribute!

@unional
Copy link
Contributor Author

unional commented Jan 17, 2024

Sounds good.

I'm also thinking about virtual keyboard.
But I'm thinking more as a tool to test and develop. i.e. a mocking keyboard.

For that, the way the code is written needs to change. e.g. define a proper interface of the keyboards and the business logic (high-level policies) only consume interfaces.

Another approach is to use inheritance, with abstract class etc (similar to just interface, but more liberate and more OO-like)

@alexpargon
Copy link
Contributor

That's a very good approach to further leverage the virtual keyboard's concept! in fact, one of the reasons to develop that feature was to test out the interface without requiring access to a physical version of the keyboard.

it would be very nice to be able to write tests against the same endpoints that are used for regular functionality. I will explore this concept and probably will create a discussion around it, so we can polish the concept 💎

@unional
Copy link
Contributor Author

unional commented Jan 27, 2024

@alexpargon how would you like to organize the tests?

I typically does not do *.test.ts(x).
I use that name to further categorize the kind of tests they are:

  • *.spec.ts: specification test that shows how the code are being used and its behavior. Basically they are unit tests that are short-range (typical unit tests) or long-range (more like integration test, but does not cross physical boundary)
  • *.unit.ts: unit test that are uninteresting to read. e.g. tests for internal functions.
  • *.system.ts: tests that cross physical boundary. e.g. tests that rely on fixtures, using file system, etc. These tests are more involving to write and also more costly to run. So they can be run selectively during full test runs, in CI, etc., and can be skipped during active development (i.e. in watch mode, yarn t)

There are other kind of tests, e.g. accept, integrate, load, stress, etc., you get the idea.

Also, I can introduce storybook, visual snapshot testing, interactive tests, etc.
Need to see if that is possible with electron app, or is there other alternatives.
These will be in separate PRs, of course.

@@ -166,7 +141,7 @@ export var arduino = {
let { address } = dataObjects[0];

if (address < 2000) {
finished(true, `You're attempting to overwrite the bootloader... (0x${padToN(num2hexstr(dataObjects[0].address), 8)})`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is simplified as num2hexstr calls padToN already.

@unional
Copy link
Contributor Author

unional commented Jan 27, 2024

The simple functions are extracted. The *_cb functions are also identical, but the whole Focus and flasher code probably need to take a deep look to see how to invert the dependencies so that it can be testable.

We can do that in another PR.

@unional
Copy link
Contributor Author

unional commented Jan 27, 2024

I have also updated the tsconfig.json to make sure is has noEmit as the code is built using electron/webpack.

Also added a stricter tsconfig for testing newer code.
It is only used during yarn test:type and not used as the default config at the moment.

We can gradually improve the code quality and switch over in the future.

@@ -26,7 +26,7 @@ export function decodeHexLine(line: string) {
const bytes = new ArrayBuffer(byteData.length);
const bytesView = new Uint8Array(bytes, 0, byteData.length);

for (let i = 0; i < byteData.length; i++) bytesView[i] = byteData[i];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is updated to better infer types.
Test has this covered.

vite.config.mts Outdated
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 file extension is changed to .mts as the CJS build for vitest is deprecated.
ESM code needs either type: module or .mts.

}
},
"include": ["src/**/*"],
"files": ["src/renderer/theme/styled.d.ts"]
Copy link
Contributor Author

@unional unional Jan 27, 2024

Choose a reason for hiding this comment

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

This line is removed because include includes it.
This is likely introduced when the file is drag or renamed on vscode.

"noUnusedLocals": true,
"noUnusedParameters": true,
"strict": true,
// "verbatimModuleSyntax": true // requires TypeScript 5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently this cannot be enabled because the project is using TypeScript 4.5.

Maybe we can upgrade it in another PR?

@unional
Copy link
Contributor Author

unional commented Jan 28, 2024

  • full rewriting of Focus.API to make it work with USB HID interfaces and better integrate the other functions like virtual keyboards.

Note that I have converted Focus to TypeScript and adding tests in #700.
We can discuss that there.

This allows quick check of typescript error without running `yarn make-dev` which is much slower
The conversion will be done in many small PRs,
so that the changes can be merged quickly,
minimizing merge conflicts for the team.
The name does not accurately match behavior.
Suggest fixing it in the future
`w` is the convention I use as I'm lazy.
Feel free to suggest otherwise
the `num2hexstr` usage actually shows that the first param is a string,
and the second param is opional based on the commented console log.

the value is defaults to 0 to better match its intent and passes type check.
@alexpargon
Copy link
Contributor

Great Job @unional ! 😄

really appreciate your help with this 👍🏻

@alexpargon alexpargon merged commit 67aed25 into Dygmalab:newFocusArch Feb 14, 2024
6 of 10 checks passed
@unional unional deleted the ts1 branch February 14, 2024 20:47
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