Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add typescript types #3

Closed
wants to merge 1 commit into from
Closed

feat: add typescript types #3

wants to merge 1 commit into from

Conversation

drwpow
Copy link

@drwpow drwpow commented Nov 8, 2020

Changes

This PR adds missing types from TypeScript, but doesn’t fix the broken ES build as that would require a breaking API change (see comment below).

Resolves #2.

Testing

This PR doesn’t change any core code as intended, so no tests need to be updated nor manual tests done

@ghost
Copy link

ghost commented Nov 8, 2020

DeepCode's analysis on #1a65c8 found:

  • ℹ️ 1 minor issue. 👇

Top issues

Description Example fixes
Array type using 'T[]' is forbidden for non-simple types. Use 'Array' instead. Occurrences: 🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@drwpow
Copy link
Author

drwpow commented Nov 8, 2020

Contrary to the direction explored in #2, I wasn’t able to convert this project to TypeScript. Because of a widely-discussed interop issue between ES Modules and CommonJS, trying to convert it results in the following breakage:

    TypeError: hypertag is not a function

The fix is it would require an API change:

- hypertag(…)
+ hypertag.parse(…)

As the original goal of this PR was to add missing types, I’m thinking maybe a separate type definition should be added, and everything else kept as-is. While that wouldn’t fix the ESM build, I don’t know how to do that without introducing a breaking API change like so 👆🏻 .

@drwpow drwpow changed the title feat: convert project to typescript feat: add typescript types Nov 8, 2020
@drwpow
Copy link
Author

drwpow commented Nov 18, 2020

@AndreasPizsa what do you think about the deepcode-ci-bot‘s suggestions? I find it weird that it‘s requesting a change to the types, because those actually were generated directly from TypeScript itself (they weren‘t handwritten). I’m more inclined to trust TypeScript‘s built-in compiler than the lint rule, but open to feedback if you have any thoughts there.

@drwpow drwpow closed this by deleting the head repository Sep 17, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript support + ESM module error
1 participant