Skip to content

Add generic prop type support #152

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

Merged
merged 5 commits into from
Apr 3, 2022

Conversation

jamesrweb
Copy link
Collaborator

Fixes issue #151

Proposed Changes

  • Added support for generic prop types

Additional notes

Please test this thoroughly with and without generics as this is a potentially breaking change and we want to be sure it is not!

Please also wait for review from @junwen-k so that he can verify the topic is resolved or needs further work / coverage.

@jamesrweb jamesrweb requested a review from yevdyko March 21, 2022 16:48
@jamesrweb jamesrweb self-assigned this Mar 21, 2022
@jamesrweb jamesrweb enabled auto-merge (squash) March 21, 2022 16:48
@jamesrweb jamesrweb linked an issue Mar 21, 2022 that may be closed by this pull request
@yevdyko yevdyko force-pushed the add-generics-support-for-prop-definitions branch from 4575b33 to c8bbd41 Compare March 21, 2022 17:54
@junwen-k
Copy link

@jamesrweb Great job and thanks for the quick responses!

I've tested locally with the new changes and it seems to be working as intended. You might want to consider updating README.md under the typescript usage section in order to improve the documentation. I can also submit a pull request if you don't mind :)

Here's an example that I used for my Next.js typescript project.

import dynamic from 'next/dynamic';

import type { P5WrapperProps } from 'react-p5-wrapper';

interface YourSketchProps {
  backgroundColor: string;
}

const ReactP5Wrapper = dynamic<P5WrapperProps<YourSketchProps>>(
  () => import('react-p5-wrapper').then((mod) => mod.ReactP5Wrapper),
  {
    ssr: false,
  }
);

const sketch: Sketch<YourSketchProps> = (p5) => {
  // ...

  p5.updateWithProps = (props) => {
    backgroundColor = props.backgroundColor; // typed
    // ...
  };
}

// ...

<ReactP5Wrapper
  sketch={sketch}
  backgroundColor="rgb(255, 255, 255)" // typed
/>

@jamesrweb jamesrweb force-pushed the add-generics-support-for-prop-definitions branch 3 times, most recently from b744a75 to c94ed78 Compare March 22, 2022 22:42
@jamesrweb
Copy link
Collaborator Author

@junwen-k @yevdyko I implemented some changes due to one missing implementation of the generic I noticed, this caused an issue which is a known problem with TypeScript itself and commented the workaround, etc. Please retest and provide feedback as necessary. Thanks for the feedback so far 🚀!

@jamesrweb
Copy link
Collaborator Author

@jamesrweb Great job and thanks for the quick responses!

I've tested locally with the new changes and it seems to be working as intended. You might want to consider updating README.md under the typescript usage section in order to improve the documentation. I can also submit a pull request if you don't mind :)

Here's an example that I used for my Next.js typescript project.

import dynamic from 'next/dynamic';

import type { P5WrapperProps } from 'react-p5-wrapper';

interface YourSketchProps {
  backgroundColor: string;
}

const ReactP5Wrapper = dynamic<P5WrapperProps<YourSketchProps>>(
  () => import('react-p5-wrapper').then((mod) => mod.ReactP5Wrapper),
  {
    ssr: false,
  }
);

const sketch: Sketch<YourSketchProps> = (p5) => {
  // ...

  p5.updateWithProps = (props) => {
    backgroundColor = props.backgroundColor; // typed
    // ...
  };
}

// ...

<ReactP5Wrapper
  sketch={sketch}
  backgroundColor="rgb(255, 255, 255)" // typed
/>

Good point about the documentation, I will update this tomorrow also, thanks for that one, good catch!

@yevdyko
Copy link
Collaborator

yevdyko commented Mar 23, 2022

@jamesrweb are you going to add the documentation in the current PR? there are also conflicts, could you resolve them?

@junwen-k
Copy link

@junwen-k @yevdyko I implemented some changes due to one missing implementation of the generic I noticed, this caused an issue which is a known problem with TypeScript itself and commented the workaround, etc. Please retest and provide feedback as necessary. Thanks for the feedback so far rocket!

I've re-tested, it seems like the only changes that needed to be made to my previous code was

import type { P5WrapperProps, SketchProps } from 'react-p5-wrapper';

interface YourSketchProps extends SketchProps {
  backgroundColor: string;
}

And everything worked the same as before.

@jamesrweb
Copy link
Collaborator Author

@jamesrweb are you going to add the documentation in the current PR? there are also conflicts, could you resolve them?

I will add them to this PR as soon as I can. 👏🏻

@jamesrweb
Copy link
Collaborator Author

jamesrweb commented Mar 26, 2022

@junwen-k @yevdyko I implemented some changes due to one missing implementation of the generic I noticed, this caused an issue which is a known problem with TypeScript itself and commented the workaround, etc. Please retest and provide feedback as necessary. Thanks for the feedback so far rocket!

I've re-tested, it seems like the only changes that needed to be made to my previous code was

import type { P5WrapperProps, SketchProps } from 'react-p5-wrapper';



interface YourSketchProps extends SketchProps {

  backgroundColor: string;

}

And everything worked the same as before.

@junwen-k Excellent! once the documentation is updated then we can get this merged then. 🚀

If there's any issues otherwise just let me know. I'll update the documentation ASAP otherwise 😇.

junwen-k
junwen-k previously approved these changes Mar 26, 2022
@jamesrweb jamesrweb force-pushed the add-generics-support-for-prop-definitions branch from c94ed78 to fa6a299 Compare March 27, 2022 20:15
@jamesrweb jamesrweb force-pushed the add-generics-support-for-prop-definitions branch from fa6a299 to 028911a Compare March 27, 2022 20:16
@jamesrweb
Copy link
Collaborator Author

jamesrweb commented Mar 27, 2022

@junwen-k @yevdyko I updated the documentation and resolved the conflicts, please take another look and approve if you are confident that:

a) There isn't a breaking change being introduced
b) The current changes resolve issue #151
c) The documentation is thorough and correct enough for release

Thanks in advance and fingers crossed we are almost there now with this feature 🎉!

@jamesrweb
Copy link
Collaborator Author

@yevdyko @junwen-k any idea when you'll have chance to review this again? ☺️

Copy link

@junwen-k junwen-k left a comment

Choose a reason for hiding this comment

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

PR LGTM :)

Copy link
Collaborator

@yevdyko yevdyko left a comment

Choose a reason for hiding this comment

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

@jamesrweb The documentation looks great, the changes are also good. I tried your examples in the typescript demo app and everything works as expected. I would say that it's ready to be released!

Thanks for this significant improvement 💪

🚢 🚢 🚢

@jamesrweb jamesrweb merged commit 2c6770a into master Apr 3, 2022
@jamesrweb jamesrweb deleted the add-generics-support-for-prop-definitions branch April 3, 2022 12:46
jamesrweb added a commit that referenced this pull request Aug 15, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Improvement of SketchProps typing using Generics?
3 participants