broccolijs / broccoli-plugin

Base class for Broccoli plugins
MIT License
25 stars 25 forks source link

Converting to typescript #33

Closed thoov closed 5 years ago

thoov commented 5 years ago

Also includes some general cleanups.

cc: @mike-north

rwjblue commented 5 years ago

(we should also make those changes to the other libs recently converted, to fix these fundamental foot gun issues)

stefanpenner commented 5 years ago

I think the one tricky part we likely need to figure out, is BroccoliPlugin vs Tree/Node interfaces. As broccoli's design doesn't impose a constraint on inputs being Plugins, rather simply being tree's.

For example, @eaf4's typings in embroider of broccoli-plugin expose 3 things Options Plugin and Tree: https://github.com/embroider-build/embroider/blob/master/types/broccoli-plugin/index.d.ts

Ultimately we could make the call, and say inputs must actually be BroccoliPlugin | string, and drop the Tree/Node concept.

stefanpenner commented 5 years ago

Also speaking with @thoov https://github.com/broccolijs/broccoli-node-api exists, and we need to consider this and using this as the baseline for these shared types.

stefanpenner commented 5 years ago

After speaking with @krisselden I think using https://github.com/broccolijs/broccoli-node-api is appropriate, but we can chat in out meeting.

stefanpenner commented 5 years ago

https://github.com/broccolijs/broccoli-plugin/pull/36 should help simplify the types

thoov commented 5 years ago

cc: @mike-north @krisselden @stefanpenner @rwjblue @oligriffiths

Could you guys take another look at this. I believe all of the feedback has been addressed and I think this is fairly close to being ready to merge.

stefanpenner commented 5 years ago

Should we concert the tests as well, ensure they can be satisfied by the available typings?

oligriffiths commented 5 years ago

Should we concert the tests as well, ensure they can be satisfied by the available typings?

I think this is a good idea, end to end typescript. Maybe for a separate PR...?

krisselden commented 5 years ago

Since tests don’t need to generate types you can still type check them without conversion.