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

fix: type getVariableValue helper #796

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

adamviktora
Copy link
Contributor

@adamviktora adamviktora commented Oct 23, 2024

Closes #790, #630

I had to add some new helpers to simplify proper type checking.

@@ -54,19 +53,23 @@ module.exports = {
const selectableActionsValue = getAttributeValue(
context,
selectableActionsProp.value
);
) as ObjectExpression["properties"];
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're having to cast the return values of getAttributeValue in mods now I feel like there's something not right going on.

I don't want to spam this comment out so echo this same thing for the below files where we're casting the return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here the casting is done because the actual selectableActions prop on CardHeader accepts only an object (CardHeaderSelectableActionsObject), so consumers shouldn't be able to pass anything else. I added a comment to make it clear.

The getAttributeValue should ultimately get to the ObjectExpression, or return undefined (e.g. if it was an Identifier imported from another file) in which case we don't do the fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But yeah on many other places, the casting was done due to the lack of typing of the getAttributeValue helper

Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

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

Great work on this so far! But I feel like for an issue focused on improving our typing we probably don't want to be adding a bunch of type casting if it can at all be avoided.

Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

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

Great changes 🚀

I'm still not loving that we're having to make assertions about the return value pretty much everywhere we're using the helper now though. I'm not sure what can be done about that.... but I feel like there should be something.

Will do a little poking at this and probably come back with an approval after I can't find anything I like more.

@adamviktora
Copy link
Contributor Author

Yeah, I don't love it either, but I think there is not a way of doing it without the type assertion.

In most cases it is being used as a way of telling the rule what type will the prop of the PatternFly react component accept.

So for example if we expect a variant value of an enum or string:
ToolbarItemVariant | 'bulk-select' | 'overflow-menu' | 'pagination' | ... we have to cast it to a string or a MemberExpression.

Another approach we could use instead of casting could be that we pass the type we expect to get back as an argument to the getAttributeValue, but we would have to probably cast anyway at some point e.g. to distinguish between an enum and a string.

In some cases the casting seems unnecessary (as Property), VSCode even doesn't mark it read, but ESLint doesn't like it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants