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: further resolve type of async components #562

Merged

Conversation

dwirz
Copy link
Contributor

@dwirz dwirz commented Dec 9, 2024

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally for affected output targets
  • Tests (npm test) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue URL: #561

What is the new behavior?

see Issue

Does this introduce a breaking change?

  • Yes
  • No

Other information

None.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Great enhancement 👏 one comment though 🤔

* @param value - the value to check
* @returns true if the value is empty, false otherwise
*/
const isEmpty = (value: any): value is null | undefined => value === null || value === undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to type the input value as unknown, same with other checks further below?

Suggested change
const isEmpty = (value: any): value is null | undefined => value === null || value === undefined;
const isEmpty = (value: unknown): value is null | undefined => value === null || value === undefined;

@dwirz
Copy link
Contributor Author

dwirz commented Dec 10, 2024

@christian-bromann I updated all the type guards to use unknown, which definitely makes sense and also makes them more robust since it requires more thorough checks. 👍

As mentioned in #563, the current PR includes everything and covers more React cases. However, since I rewrote the entire function and there aren’t as many tests as in the Stencil repo, I can’t be entirely sure I’ve covered all cases. That said, I’m pretty confident—all cases of ReactNode-typing are handled. The only potential issue is with the _payload stuff.

So feel free to either close this and merge the other PR or vice versa. Up to you – either change would solve the issue. 🙂

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@christian-bromann christian-bromann merged commit 1b8fb07 into ionic-team:main Dec 10, 2024
3 checks passed
@christian-bromann
Copy link
Member

I am planning to release all output targets today or tomorrow. Thanks again for your contribution!

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.

2 participants