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(types): modify file suffix after build types so that it can export correct types #6051

Merged
merged 4 commits into from
May 29, 2024

Conversation

dongnaebi
Copy link
Contributor

This PR contains:

  • A BUGFIX

Describe the problem you have without this PR

fix: #6022

Todos

  • Tests
  • Documentation
  • Typings
  • Changelog

@pubkey
Copy link
Owner

pubkey commented May 26, 2024

@dongnaebi this fails the CI, please check.

@dongnaebi
Copy link
Contributor Author

Sorry I didn't notice this, it seems to be because I only exported the type. I removed the type, and the test:typings command has been tested successfully. I don't know how to test other CI, please run CI again to check.

@dongnaebi
Copy link
Contributor Author

Do you mind if I add a line // @ts-nocheck to dist/types/index.d.ts?

@pubkey
Copy link
Owner

pubkey commented May 27, 2024

I am not sure if we are working in the correct direction. Is this still correct typescript code which is generated? Why do we need ts-nocheck?

@dongnaebi
Copy link
Contributor Author

Yes, in the long run this is not the right direction, as I mentioned at the beginning, this is just the solution with the minimum cost to make it work.

I am not very familiar with TSC compilation. Perhaps we should compile JS files and .d.ts files into the same directory. I usually directly use tools like rollup or esbuild to compile code and generate .d.ts files. Maybe we should spend some time trying it out.

The two commits today were both for passing CI tests(ts-nocheck is for npm run check-types); currently, the generated code works fine.

@pubkey pubkey merged commit 7827350 into pubkey:master May 29, 2024
20 checks passed
pubkey added a commit that referenced this pull request May 29, 2024
@pubkey
Copy link
Owner

pubkey commented May 29, 2024

Ok, I merged this for now. In the future I want to drop the cjs export which simplifies the setup a lot and then maybe there is a better solution.

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.

Can't resolve types
2 participants