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

Connect as a functional component #150

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tdawes
Copy link
Member

@tdawes tdawes commented Oct 4, 2019

Please test before merging - particularly when importing @prodo/core from outside of the monorepo.

});

Object.keys(prevWatched).forEach(pathKey => {
const keyDeleted = watched.current.hasOwnProperty(pathKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

BUG: missing !

Copy link
Member Author

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sorry, you mean negation. Good spot!


public render() {
this.createViewCtx();
return (func as ((args: any) => (props: any) => any))(ctx)(props);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this breaks support for hooks because func(ctx) will be a different function each time.

Copy link
Member Author

Choose a reason for hiding this comment

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

We've done this before and I think it worked. Will do some testing

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think hooks still work. At least the update-testing example works as expected.

@tdawes
Copy link
Member Author

tdawes commented Oct 4, 2019

Looks like this breaks the dev tools though - looking now but please don't merge.

logger.debug("store", store);
}, []);

const pathNodes = React.useRef({});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this by typed?

Comment on lines +5 to +7
// const getValue = (path: string[], obj: any): any =>
// path.reduce((x: any, y: any) => x && x[y], obj);

Copy link
Collaborator

Choose a reason for hiding this comment

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

🔫

Comment on lines +40 to +44

// if (seenValue !== getValue(path, store.universe)) {
// console.log(`Not seen ${path} before, so rerendering`);
// node.forceUpdate();
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔫


return true;
};
const getValue = (path: string[], obj: any): any =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the same as the get function in watch.ts. Maybe extract so its only defined once.

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.

3 participants