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: update types to be compatible with v10 #680

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

wolfy1339
Copy link
Contributor

No description provided.

@wheresrhys
Copy link
Owner

Thanks for this

@wheresrhys wheresrhys merged commit aba4e0b into wheresrhys:main Jun 18, 2024
1 check failed
@wolfy1339 wolfy1339 deleted the patch-1 branch June 18, 2024 14:49
@wolfy1339
Copy link
Contributor Author

Unfortunately, your further commit ae15739 has broken the types again.

Now the package is unusable in ESM with TypeScript currently

It seems that there needs to be types for both ESM and CJS

@wheresrhys
Copy link
Owner

wheresrhys commented Jun 18, 2024 via email

@wolfy1339
Copy link
Contributor Author

The CI case handles CJS, and can be updated for dual-support. I get that it broke, it can be fixed

We use fetch-mock all over the @octokit org for the JS packages

Specifically this repo: octokit/core.js#686
It might not be the smallest, but it show the issue and how the issue is not present before

The error I am getting is the following:

test/issues.test.ts:2:8 - error TS1192: Module '"/home/runner/work/core.js/core.js/node_modules/fetch-mock/types/index"' has no default export.

Since the types only have CJS exports, TS complains. Runtime is fine

@wolfy1339
Copy link
Contributor Author

I created a minimal reproducible test case, https://stackblitz.com/edit/fetch-mock-680?file=index.ts

@wheresrhys
Copy link
Owner

Thank you

@wheresrhys
Copy link
Owner

I think this has fixed the tests and I've reverted to your original change too https://github.com/wheresrhys/fetch-mock/blob/main/types/fetch-mock-tests.ts#L1C1-L1C28

Thanks for the pointers

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