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

Add types to compat #1609

Merged
merged 16 commits into from
May 8, 2019
Merged

Add types to compat #1609

merged 16 commits into from
May 8, 2019

Conversation

pmkroeker
Copy link
Contributor

@pmkroeker pmkroeker commented May 6, 2019

This is still a WIP of adding export types with preact/compat for those that wish to use import things directly through compat.

Some things are still being hashed out, but wanted some feedback.

Related #1608

* Map and for each abstraction
Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

Awesome, this is a really great start 👍 💯 All types are there and we just need to make sure we're consistent and keep the types for props 👍 One nit: We don't have a space after a function name in our whole codebase: function foo () {} should be formatted like function foo() {}. Left a few comments with ideas for more type-safe declarations. Let me know what you think 🙂

@pmkroeker
Copy link
Contributor Author

@marvinhagemeister thanks. Ya this is my first foray into adding types to a larger package. And I'll fix the spacing thing too!

@pmkroeker
Copy link
Contributor Author

I just changed over from using PreactElement to Element. I pulled some of the initial typings from the JSDoc comments, but it seems like some of the places that use PreactElement in fact require just Element (mostly due do passing something like document.querySelector('#root').

@marvinhagemeister does this seem ok?

@pmkroeker pmkroeker marked this pull request as ready for review May 7, 2019 15:35
@coveralls
Copy link

coveralls commented May 7, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling e7286df on pmkroeker:master into 4aabebb on developit:master.

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Nice, This is looking great 👍

@marvinhagemeister
Copy link
Member

It's coming along very well! I'd just add the default export like mentioned in the comments and than it's ready to be merged in my opinion 👍

@pmkroeker
Copy link
Contributor Author

I did some testing and it seems as though the way the index.js file is created, TS is able to map the definitions to a default export as well.

import React, { createPortal } from 'preact/compat';

React.createPortal(); // gives missing args, vnode not provided
createPortal(); // same message as above

Both of those behave the same, plus intellisense over React shows all the appropriate methods.

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

Sweet ⚡✔️

@marvinhagemeister marvinhagemeister merged commit b1a1956 into preactjs:master May 8, 2019
@pmkroeker pmkroeker mentioned this pull request May 8, 2019
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.

None yet

5 participants