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

feat: declaration style #76

Merged
merged 10 commits into from
Sep 16, 2023
Merged

Conversation

marcagba
Copy link
Contributor

@marcagba marcagba commented Sep 2, 2023

Description

This PR introduce an option to transform prop-types into type aliases instead of interfaces.

How it works

A --prefer-type-aliases flag is introduced, which will then toggle type aliases generation in place of interfaces.
When the flag is unused interfaces are emitted like before the patch.

Screenshot

Screenshot 2023-09-02 at 20 09 29

Meta

Checklist

  • unit test are up to date
  • e2e are up to date
  • Readme is up to date
  • Online playground is up to date

README.md Outdated
@@ -164,3 +165,39 @@ MyComponent.propTypes = {
},
}
```

### `--prefer-type-aliases`
Copy link
Owner

Choose a reason for hiding this comment

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

I think I would prefer to use an option of --declaration-style with the default value being interface.

Suggested change
### `--prefer-type-aliases`
### `--declaration-style=type`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

README.md Outdated
@@ -13,6 +13,7 @@ Codemod to convert React PropTypes to TypeScript types.
- Supports files with multiple components
- Copies JSDoc comments to the generated TypeScript types
- Option to remove or preserve PropTypes after converting to TS
- Choose between interface and type alias
Copy link
Owner

Choose a reason for hiding this comment

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

Not a important feature for the highlights section.

Suggested change
- Choose between interface and type alias

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

@@ -0,0 +1,11 @@
import PropTypes from "prop-types"
Copy link
Owner

Choose a reason for hiding this comment

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

One test is plenty, definitely don't need to duplicate every single test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

transform.ts Outdated
@@ -260,7 +280,9 @@ function addFunctionTSTypes(
componentTypes: CollectedTypes
) {
source.forEach((path) => {
const typeName = createInterface(path, componentTypes)
const typeName = options.preferTypeAliases
? createTypeAlias(path, componentTypes)
Copy link
Owner

Choose a reason for hiding this comment

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

Let's just rename createInterface to something more generic and do this as a conditional in that function. Don't want all the duplicate code this currently has.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

@marcagba marcagba changed the title feat: prefer type aliases feat: declaration style Sep 10, 2023
j.identifier(typeName),
j.tsInterfaceBody(types.types.map(createPropertySignature))
)
options.declarationStyle === "type"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only check if the value is type. In this way, in case of interface or unaccepted value the default will be to output interface.
LMK if this behavior is ok or if you'd rather have a throw or else in case of not listed declaration style value

Copy link
Owner

Choose a reason for hiding this comment

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

This is perfect!

Comment on lines +95 to +101
// --declaration-style=type
defineTest(
__dirname,
transform,
{ "declaration-style": "type" },
"declaration-style-type"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Way less tests than in the first draft ;)
LMK if you'd like some additional ones

Copy link
Owner

Choose a reason for hiding this comment

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

Nope, this seems fine.

}

h1 {
font-size: 22px;
margin: 0;
margin-right: auto;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small css update to align options at the end of their container
screenshot:
Screenshot 2023-09-10 at 19 15 38

@marcagba
Copy link
Contributor Author

Thank you for your feedbacks @mskelton !
I addressed all of those, let me know if you see anything else

@mskelton mskelton merged commit ef1771c into mskelton:main Sep 16, 2023
3 checks passed
@mskelton
Copy link
Owner

Thanks for those updates, great work on this feature!!!

@marcagba marcagba deleted the feat/prefer-type-aliases branch March 22, 2024 23:35
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