Kinrany / vue-p5

Vue component for p5.js
125 stars 16 forks source link

feat: add unit tests for the framework #35

Open sniperadmin opened 3 years ago

sniperadmin commented 3 years ago

This PR should do the following:

Kinrany commented 3 years ago

Thanks for the PR!

Could you add some motivation for it? I'm particularly interested in the code coverage integration and the specific tests you chose.

I want to err on the side of accepting all changes that improve something for the users and contributors, as long as the repo is small and easy to understand: vue-p5 is not a big or particularly well-maintained library, so I'd like to make sure anyone can fork it and change for their needs.

sniperadmin commented 3 years ago

Sounds perfect!

I do agree to think and adapt what's necessary to achieve your target, so let me try adding more cases, maybe using some functional programming and setup reusable modules I guess

I do still have lots of random thoughts in my mind, hopefully I can organize them here ;)

Kinrany commented 3 years ago

Let's take a step back. In what way would these changes improve the library for you?

For example, as a user I'd like to avoid typos in event names. So I might add TypeScript support so that all typos are caught at design time. This makes me as a contributor want to change how the build tooling is set up. So I might add a unit test to be confident that the build output still works in the browser.

To clarify, I'm not suggesting these specific changes. I want to know what part of the user/contributor experience will be improved by the changes you're suggesting.

sniperadmin commented 3 years ago

I actually do love your feedback because it helps me organizing my thoughts a bit I do agree of course on what's been stated in your comment.

let me mindmap my thoughts one more time and take those points into concideration

sniperadmin commented 3 years ago

Type-script has nailed it 🎉

New advantages:

The Pros and Cons.

Pros:

Cons

Next up:

Kinrany commented 3 years ago

We cannot say VueP5.something.. The user can use the original p5 itself as an argument in the sketch event

This is a big cons: the p5 reexport is there specifically to expose p5 outside of a sketch context. Inside a sketch context the sketch object is a replacement for the p5 object.

sniperadmin commented 3 years ago

Good Call, I tried actually to use the previous reexport to call consts or something like p5.Vector, but got undefined, any thoughts around this?

if is it a buggy behavior with the reexport itself, how should we test that?

Kinrany commented 3 years ago

I never used it in any of the examples, so it may be broken indeed!

If it is, this is probably a build tooling issue. The test would have to use build output as a dependency and run an equivalent of

const VueP5 = require('vue-p5');
console.assert(VueP5);
console.assert(VueP5.p5);
console.assert(VueP5.p5.abs);
sniperadmin commented 3 years ago

Here is an issue if I tried to use the reexported p5:

sketch(sk: any): void {
  const p5 = new sk.p5()
}
TypeError: sk.p5 is not a constructor

      56 |       methods: {
      57 |         sketch(sk: any): void {
    > 58 |           const p5 = new sk.p5()
         |                     ^
      59 |         }
      60 |       }
      61 |     })