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: ensure lazy typedefs mirror defaults #1153

Merged
merged 2 commits into from
Jan 24, 2021

Conversation

jahed
Copy link
Contributor

@jahed jahed commented Jan 23, 2021

Fixes #1146

Notes

To avoid the cyclic dependency (which would cause an invalid lazy -> base -> lib dependency), since base.d.ts is used internal only, it's safe to move the default types there (to avoid base -> lib) and re-export them in index.d.ts.

I also moved index.d.ts to lib/index.d.ts as that's where lib/index.js is. So main and typings match to avoid confusion. This also solves the relative path issue when copying it for lazy/index.d.ts.

@JuanM04
Copy link
Contributor

JuanM04 commented Jan 23, 2021

I also moved index.d.ts to lib/index.d.ts as that's where lib/index.js is. So main and typings match to avoid confusion. This also solves the relative path issue when copying it for lazy/index.d.ts.

That's pretty neat

To avoid the cyclic dependency (which would cause an invalid lazy -> base -> lib dependency), since base.d.ts is used internal only, it's safe to move the default types there (to avoid base -> lib) and re-export them in index.d.ts.

You just changed where the cyclic dependency happens: now, for example,soundcloud.d.ts imports base.d.ts and vice-versa.
Maybe, you should create a types/lazy/index.d.ts like this:

export * from '../index'
export { default } from '../index'

@jahed jahed force-pushed the fix/1146-lazy-typedefs branch from 35fab91 to 717185d Compare January 23, 2021 22:56
@jahed
Copy link
Contributor Author

jahed commented Jan 23, 2021

You just changed where the cyclic dependency happens

Ah, of course. 🤦🏽 I'll avoid resolving the cyclic dependency. Thought it was causing errors so went for a quick "fix" but it was actually npm link anyway. (microsoft/TypeScript#6496 (comment))

I've pushed an update.

types/lazy/index.d.ts Outdated Show resolved Hide resolved
@jahed jahed force-pushed the fix/1146-lazy-typedefs branch from 717185d to f46f22d Compare January 23, 2021 23:05
package.json Outdated Show resolved Hide resolved
Co-authored-by: Pete Cook <pete@cookpete.com>
@jahed jahed requested a review from cookpete January 23, 2021 23:22
@cookpete cookpete merged commit 0cc1b39 into cookpete:master Jan 24, 2021
@cookpete
Copy link
Owner

Great work @jahed. Thanks for the help @JuanM04 👍

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.

TypeScript declarations for react-player/lazy broken in v2.8.1
3 participants