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

Export Props interfaces to allow exported declarations #236

Merged
merged 2 commits into from
Dec 31, 2019

Conversation

nealeu
Copy link
Contributor

@nealeu nealeu commented Dec 31, 2019

Otherwise we get:

TS4023: Exported variable 'MyAsync' has or is using name 'PendingProps' from external module "/node_modules/react-async/dist-types/index" but cannot be named.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 31, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5ffaab2:

Sandbox Source
still-tdd-pj359 Configuration

@codecov
Copy link

codecov bot commented Dec 31, 2019

Codecov Report

Merging #236 into next will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             next     #236   +/-   ##
=======================================
  Coverage   98.58%   98.58%           
=======================================
  Files           8        8           
  Lines         424      424           
  Branches      148      148           
=======================================
  Hits          418      418           
  Misses          6        6
Impacted Files Coverage Δ
packages/react-async/src/Async.tsx 100% <ø> (ø) ⬆️

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 1351fd0...5ffaab2. Read the comment docs.

Otherwise we get:

TS4023: Exported variable 'MyAsync' has or is using name 'PendingProps' from external module "<snip>/node_modules/react-async/dist-types/index" but cannot be named.
Copy link
Member

@phryneas phryneas left a comment

Choose a reason for hiding this comment

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

It wouldn't hurt exposing those, so we can definitely do that.

But could you please share in which scenario you need those? Are you creating your own library with declarations wrapping react-async? Just for usage, I believe this would not be neccessary.

@nealeu
Copy link
Contributor Author

nealeu commented Dec 31, 2019

Thanks for the feedback @phryneas.

I've got a monorepo where we're using Typescript project references and incremental builds which therefore generate declaration files.

I've decided to separate the Async from the promiseFn, such that components can consume an Async without the implementation of the fetch also being in that library. This allows us to build our components independently of the data source (which could be IndexedDB or a remote system).

So we have the following code in AsyncSessionData.tsx:

export const AsyncSessionData = createInstance<Context>({}, "SessionData");

Which results in the following in AsyncSessionData.d.ts:

export declare const AsyncSessionData: import("react-async").AsyncConstructor<Context>;

Which my previous PR would have solved had it not been that AsyncConstructor uses PendingProps etc.

I've now got this behaving as needed with the exports in this PR

@nealeu nealeu marked this pull request as ready for review December 31, 2019 17:36
@phryneas
Copy link
Member

Okay, that seems like a sensible use case.
Usually, I'm a bit against exposing too many internal types, as it gets in the way of refactoring without causing breaking changes, but this seems good from my side. :)

I take it you validated the codesandbox build now checks all your requirements? :)

@nealeu nealeu changed the title Export Props interfaces for to allow exported declarations Export Props interfaces to allow exported declarations Dec 31, 2019
@nealeu
Copy link
Contributor Author

nealeu commented Dec 31, 2019

I take it you validated the codesandbox build now checks all your requirements? :)

Yes. Currently have our build running successfully against the sandbox build for this PR.

@ghengeveld
Copy link
Member

LGTM. It'll mean we have to do a major release when changing any of these, but I suspect that would be the case anyway.

@ghengeveld ghengeveld merged commit 69e0019 into async-library:next Dec 31, 2019
@ghengeveld
Copy link
Member

@all-contributors please add @nealeu for code.

@allcontributors
Copy link
Contributor

@ghengeveld

I've put up a pull request to add @nealeu! 🎉

@nealeu nealeu mentioned this pull request Mar 25, 2020
@ghengeveld ghengeveld mentioned this pull request Mar 27, 2020
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