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

Update typescript definition for resolve. #63

Closed
wants to merge 1 commit into from

Conversation

dawnmist
Copy link

Expanding the resolve definition allows it to work with react-redux's connected components. Addresses issue #62.

Expanding the resolve definition allows it to work with react-redux's connected components.
@codecov
Copy link

codecov bot commented Oct 13, 2017

Codecov Report

Merging #63 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #63   +/-   ##
=======================================
  Coverage   94.38%   94.38%           
=======================================
  Files           3        3           
  Lines          89       89           
  Branches       25       25           
=======================================
  Hits           84       84           
  Misses          5        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94fc382...a209724. Read the comment docs.

@ctrlplusb
Copy link
Owner

Thanks for looking into this @dawnmist - my typescript knowledge is zil so I have been depending on help from others with this.

Before we consider merging this I'd like to get @bdoss's input here as his PR changed the resolve from:

resolve: () => Promise<React.Component<P>>;

to:

resolve: () => Promise<React.ComponentType<P>>;

It seems to me that returning ReactNode sounds good as it's a subset of the Component types right but would like a second opinion. :)

@bdoss
Copy link

bdoss commented Oct 13, 2017

I think this is due to the autoResolveES2015Default option of the asyncComponent definition. It defaults to true, so it's hiding that implementation detail from the TypeScript compiler. I've been manually resolving like this:

resolve: () => import('MyComp').then(x => x.default)

It keep the compiler happy so it doesn't think you're trying to do anything silly. The advantage with using ComponentType<T> being that when you import the async component elsewhere for use as a JSX element, the TypeScript compiler can check and give suggestions on props.

Edit: I gave another look over your mods to the type definition and see that with your change to the resolve method you would still gain the ability to get prop checks/suggestions (if you explicitly provide your props Type) when importing the async component, while sacrificing the ability to properly type check the resolve callback itself. This would allow interoperability with the autoResolveES2015Default as you've demonstrated, but albeit with a trade off on a more generic (and I'm not sure semantically correct) definition.

@dawnmist dawnmist closed this Oct 14, 2017
@dawnmist dawnmist deleted the resolve-return-type branch October 14, 2017 01:56
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.

3 participants