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 enhanced typings #1287

Merged
merged 8 commits into from
Mar 17, 2018
Merged

Add enhanced typings #1287

merged 8 commits into from
Mar 17, 2018

Conversation

as-com
Copy link
Contributor

@as-com as-com commented Feb 25, 2018

Objective
This PR adds enhanced typings for JSX as well as a number of other tweaks to enable Inferno to work with TypeScript's strict mode enabled.

Typings were copied from @types/react and bashed on until it and an example project compiled. Types and interfaces were modified to fit Inferno's internal types, classes were removed, and unimplemented features were commented out. Please feel free to rearrange/rewrite any of the type definitions I wrote if you have more knowledge of Inferno and/or TypeScript.

Closes Issue
It closes issue #686

@coveralls
Copy link
Collaborator

coveralls commented Feb 25, 2018

Coverage Status

Coverage increased (+0.9%) to 95.796% when pulling 0bba495 on as-com:enhance-typings into 4333ca7 on infernojs:dev.

export interface ComponentClass<P = {}> {
new (props?: P, context?: any): Component<P, {}>;

propTypes?: ValidationMap<P>;
Copy link
Member

Choose a reason for hiding this comment

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

Inferno does not have propTypes or contextTypes

export type InfernoChildren = string | number | boolean | undefined | VNode | Array<string | number | VNode> | null;

export interface Props {
children?: InfernoChildren;
ref?: Ref | null;
key?: any;
className?: string;
[k: string]: any;
// [k: string]: any;
Copy link
Member

Choose a reason for hiding this comment

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

remove commented out code?

React projects that don't include the DOM library need these interfaces to compile.
React Native applications use React, but there is no DOM available. The JavaScript runtime
is ES6/ES2015 only. These definitions allow such projects to compile with only `--lib ES6`.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this for inferno?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove the reference to global, there are errors with HTMLWebViewElement. Also this would be needed for projects that use Inferno on the server side.

@@ -33,10 +42,3852 @@ if (process.env.NODE_ENV !== 'production') {
}
}


Copy link
Member

Choose a reason for hiding this comment

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

these type declarations could be in its own file ?

@as-com as-com force-pushed the enhance-typings branch from 3d03819 to 095cfd2 Compare March 1, 2018 21:24
@as-com
Copy link
Contributor Author

as-com commented Mar 1, 2018

FYI, I refactored the event typings to fit more with Inferno and rebased my PR on the current dev branch.

I'm pretty sure there will be more cleanup/simplification necessary before this can be merged. I'll keep working on this maybe tomorrow.

@Havunen
Copy link
Member

Havunen commented Mar 4, 2018

There is issue related to typings: #1295 , Could that be taken care of same time?

@as-com
Copy link
Contributor Author

as-com commented Mar 11, 2018

Welp, looks like "tomorrow" turned into "next week..." :P

I think this PR is ready for a final review and merge.

@@ -275,7 +275,7 @@ class PureComponent<P, S> extends Component<P, S> {
class WrapperComponent<P, S> extends Component<P, S> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can neaten this up by having

interface ContextProps {
    context: any;
}

type WrappedComponentProps<OriginalProps> = OriginalProps | ContextProps

class WrappedComponent<P, S> extends Component<WrappedComponentProps<P>, S> {

@Havunen
Copy link
Member

Havunen commented Mar 13, 2018

@as-com What is The status of this PR. We will need to release V5 because of breaking change in Webpack compatibility. Including this there would be nice

@as-com
Copy link
Contributor Author

as-com commented Mar 13, 2018

@Havunen This PR is ready for merging, unless you find anything else that needs to be changed. 😄

@Havunen
Copy link
Member

Havunen commented Mar 13, 2018

How does this global.d.ts file work with our index.d.ts file? When you build the solution using npm run build it will auto generate index.d.ts file from source code and put that into dist folder. I'm not that familiar with typescript system, sorry if its silly question.

@Havunen
Copy link
Member

Havunen commented Mar 13, 2018

Could this issue be solved too in this PR? #1209

@as-com
Copy link
Contributor Author

as-com commented Mar 13, 2018

@Havunen #1209 should be fixed by this PR.

On a second thought, the reference to global.d.ts may not be needed. I'll look into it later.

@Havunen Havunen requested a review from deamme March 17, 2018 11:28
@@ -42,7 +42,7 @@ export function invariant(condition, format, a?, b?, c?, d?, e?, f?) {
const ARR = [];

export const Children = {
forEach(children: any[], fn: Function): void {
forEach(children: any, fn: Function): void {
Copy link
Member

Choose a reason for hiding this comment

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

Typing seems to be more correct with any[] as before?

@deamme
Copy link
Member

deamme commented Mar 17, 2018

You may need to run npm run prettier:src:ts.

@Havunen
Copy link
Member

Havunen commented Mar 17, 2018

Ok. I think this is good to go then! Lets merge it and go for v5!

@Havunen
Copy link
Member

Havunen commented Mar 17, 2018

Thank you for your work @as-com

@Havunen Havunen merged commit d7f0408 into infernojs:dev Mar 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants