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 builder components helpers to ts #15564

Merged
merged 13 commits into from
Feb 19, 2025
Merged

Conversation

adrinr
Copy link
Collaborator

@adrinr adrinr commented Feb 17, 2025

Description

Convert builder components helpers to ts

Copy link

qa-wolf bot commented Feb 17, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

@github-actions github-actions bot added firestorm Data/Infra/Revenue Team size/m labels Feb 17, 2025
@adrinr adrinr marked this pull request as ready for review February 18, 2025 08:46
Copy link
Contributor

@deanhannigan deanhannigan left a comment

Choose a reason for hiding this comment

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

Everything looks grand, just some minor comments some of the optional function parameters.

BUG - when dragging a new component to the screen I get

  • TypeError: Cannot read properties of null (reading '_children')

return searchComponentTree(rootComponent, comp => comp._component === type)
}

/**
* Recursively searches for the parent component of a specific component ID
*/
export const findComponentParent = (rootComponent, id, parentComponent) => {
export const findComponentParent = (
rootComponent: Component | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these all be set as optional?

Copy link
Member

Choose a reason for hiding this comment

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

rootComponent and id shouldn't be optional (or even allowed to be undefined) but it's just because the code that calls this probably isn't all TS - so we can't guarantee that we're not passing in something undefined by mistake. The 3rd param is optional, since it's intended to be blank when called externally, but it's used when it invokes itself via recursion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As @aptkingston mentioned this is reflecting the current usages... Most of them should not be nullable, but until everything is fully typed will not be safe to refactor

export const findComponentPath = (rootComponent, id, path = []) => {
export const findComponentPath = (
rootComponent: Component,
id: string | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, an optional string for the id param instead of string | undefined

componentId,
selector
rootComponent: Component,
componentId: string | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible optional value

@@ -1,3 +1,5 @@
// TODO: analise and fix all the undefined ! and ?
Copy link
Contributor

Choose a reason for hiding this comment

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

🥚 Just checking if this was intentionally left in

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this was left intentionally as it will require a much bigger refactor

Copy link
Member

@aptkingston aptkingston left a comment

Choose a reason for hiding this comment

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

Conversion looks good but just a query about the types 👍

Copy link
Contributor

@deanhannigan deanhannigan left a comment

Choose a reason for hiding this comment

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

LGTM!

@adrinr adrinr merged commit 8f86260 into master Feb 19, 2025
20 checks passed
@adrinr adrinr deleted the ts/builder-components-helpers branch February 19, 2025 09:25
@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
firestorm Data/Infra/Revenue Team size/m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants