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

Fixed TS issues when used with single-spa-react #4

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

Conversation

alexislucas-toast
Copy link

@alexislucas-toast alexislucas-toast commented Oct 8, 2021

Description

Hi there! 👋

For a private project, we're using the single-spa-react package, and wanted to use this new helper package to not have to reproduce the logic to mount a parcel. It's working as expected, but as we are using Typescript for this project, it is complaining that the definitions for domElementGetter between single-spa-react and dom-element-getter-helpers don't match. For reference, here's the definition from single-spa-react. I think the React ecosystem is the only one that passes props to this function.

Since the domElementGetter now takes an optional props parameter, it should still work as expected for other ecosystems that don't pass it. We've tested it in our project and the Typescript issue has disappeared.

All the tests are passing, there's no formatting issue and the project builds successfully.

Also, thank you for single-spa and all the helpers, it's awesome!

Changes

  • Updated domElementGetter function to take optional props

Copy link
Member

@joeldenning joeldenning 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 the clear description and for contributing a fix. I've left a comment below that I think will make dom-element-getter-helpers and single-spa-react types even better, but am not 100% about it and might have missed something. Let me know what you think.

interface HelperOpts {
domElementGetter?(): HTMLElement;
interface HelperOpts<ExtraProps = {}> {
domElementGetter?(props?: ExtraProps): HTMLElement;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm I think this should be more than just the extra props - also the AppProps. This is because the single-spa props get merged with the extra props to form the final props.

Suggested change
domElementGetter?(props?: ExtraProps): HTMLElement;
domElementGetter?(props?: ExtraProps & AppProps): HTMLElement;

See code below for AppProps.

https://github.com/single-spa/single-spa/blob/e63af429ad676fa2468af08b076ee154ebaa42be/typings/single-spa.d.ts#L12

Looking at the single-spa-react types, I'm not sure it's doing that part correct either, with RootComponentProps not including the sinlge-spa AppProps. So it's possible that updating the code in dom-element-getter-helpers to use AppProps causes issues with single-spa-react, in which case I think I'd prefer trying to fix both places.

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