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

Use Typescript #71

Merged
merged 34 commits into from
Jan 16, 2024
Merged

Use Typescript #71

merged 34 commits into from
Jan 16, 2024

Conversation

MyIgel
Copy link
Member

@MyIgel MyIgel commented Nov 10, 2023

Description

This replaces most .js files with their typescript equivalent.

Motivation and Context

Having no clue what something does is bad, this fixes it b having types and thus at least some clues 🎉

How Has This Been Tested?

On my laptop, technically should produce the same JS as before

Screenshots/links:

n/a

Checklist:

  • My code follows the code style of this project. (CI will test it anyway and also needs approval)
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.

@MyIgel MyIgel added the enhancement New feature or request label Nov 10, 2023
@MyIgel
Copy link
Member Author

MyIgel commented Nov 10, 2023

The only anoyance is the fact that forcegraph.js's transformPosition sets the x, y, & k parameters which are, according to the typescript doc, constants and thus triggers ide warnings.
My attempts at fixing that where not successful, the following change produces random jumps (looks like jumping back to the state before the node was selected?) when clicking around a node in graph view after highlighting it:

transform = new ZoomTransform(p.k, p.x, p.y);
draw.setTransform(transform);
redraw();

Maybe @weeman1337 has an idea here?

Copy link
Collaborator

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

🥳 thanks for migrating the project to TypeScript. Left some notes.

Also ESLint explodes, because it cannot parse TypeScript:

  • npm i --save-dev @typescript-eslint/parser
  • Add "parser": "@typescript-eslint/parser" to .eslintrc.json

lib/about.ts Outdated Show resolved Hide resolved
lib/about.ts Outdated Show resolved Hide resolved
lib/container.ts Outdated
tag = "div";
}

let self = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If a variable is not reassigned, it should be declared as const
(counts for other places as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to change as little as needed here to not make the changes more confusing than they already are but can change it at least for the self's

lib/filters/hostname.ts Outdated Show resolved Hide resolved
@@ -42,64 +54,69 @@ export const ForceGraph = function (linkScale, sidebar) {
draw.setMaxArea(canvas.width, canvas.height);
}

function transformPosition(p) {
function transformPosition(p: { k: number; x: number; y: number }) {
// @ts-ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

To solve this ignore transform should be replaced with a new instance

transform = new d3Zoom.ZoomTransform(p.k, p.x, p.y);

Copy link
Member Author

Choose a reason for hiding this comment

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

I would love doing that but that breaks it even more, see the comments above.
Can you eventually show me exactly how you mean that at some point?


if (node !== undefined) {
// @ts-ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is node actually something different here?

Copy link
Member Author

@MyIgel MyIgel Jan 9, 2024

Choose a reason for hiding this comment

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

Is should be some kind of MapNode that has an o attribute with the actual node

lib/gui.ts Outdated Show resolved Hide resolved
@@ -26,9 +36,12 @@ export const Main = function (sidebar, linkScale) {

el.scrollIntoView(false);
el.classList.add("infobox");
// @ts-ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

If destroy() should be kept with the element, you could use an extended element with the destroy() function defined.

h2.classList.add("proportion-header");
h2.textContent = _.t(heading);
h2.onclick = function onclick() {
// @ts-ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

renderSingle() should return an extended table with the elm prop.

@MyIgel MyIgel requested a review from weeman1337 January 9, 2024 14:55
Copy link
Collaborator

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Tested, looks good and works!

@weeman1337 weeman1337 merged commit 2489727 into main Jan 16, 2024
1 check passed
@weeman1337 weeman1337 deleted the typescript branch January 16, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants