Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Add typescript typings for preact #416

Merged
merged 5 commits into from May 9, 2018

Conversation

aaronjensen
Copy link
Contributor

@aaronjensen aaronjensen commented May 1, 2018

What:

These are working typescript types for Preact. What doesn't work is the build.
To make it work, the preact/package.json would need to have an entry for
typings and the typings would need to be copied to the preact folder. Since
all of this seems it would need to happen in kcd-scripts maybe @kentcdodds has
some time to help with that?

{
  "main": "dist/glamorous.cjs.js",
  "jsnext:main": "dist/glamorous.esm.js",
  "module": "dist/glamorous.esm.js",
  "typings": "typings/glamorous.d.ts"
}

Why:

There are currently no typescript types for preact

How:

I upgraded a medium sized preact-glamorous project.

Checklist:

  • Documentation N/A
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@Ailrun
Copy link
Contributor

Ailrun commented May 1, 2018

Thank you for your pull request!
The changes what you made in this PR are

  1. Copy-and-paste original types
  2. Change react to preact
  3. Change react only types(React.Component...) to preact types(preact.Component...)
  4. Add IntrinsicAttributes and IntrinsicClassAttributes in the place of JSX.{something}.

Am I right? Are there anything important I missed?
If what I listed are all, it looks like the only thing this PR missed is adding some tests... of course, after fixing typing-loading issues.

@kentcdodds
Copy link
Contributor

What doesn't work is the build.

I don't think I want to bake that into kcd-scripts just yet. So let's add a postbuild step that calls into a node script to modify the generated preact/package.json. Should be just three or four lines.

@codecov-io
Copy link

codecov-io commented May 1, 2018

Codecov Report

Merging #416 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #416   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         177    177           
  Branches       50     50           
=====================================
  Hits          177    177

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 3f03298...be86ad7. Read the comment docs.

@aaronjensen
Copy link
Contributor Author

Am I right? Are there anything important I missed?

Yeah I think that's pretty much it.

If what I listed are all, it looks like the only thing this PR missed is adding some tests... of course, after fixing typing-loading issues.

Added tests and fixed build as @kentcdodds suggested.

Let me know if there's anything else I should do. Thanks!

Copy link
Contributor

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

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

Sorry for delay. This looks OK for me.

@kentcdodds kentcdodds merged commit f556645 into paypal:master May 9, 2018
@aaronjensen aaronjensen deleted the preact-typings branch May 10, 2018 01:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants