-
Notifications
You must be signed in to change notification settings - Fork 8
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
Introduce plugin signatures #39
Conversation
Also, this commit fixes an issue with TS auto imports including weird paths that weren't specified in the tsconfig.json (top-level entries of `src`)
Preview URLsGH Env: preview |
507b62e
to
206db07
Compare
// A Real plugin | ||
////////////////////////////////////////////// | ||
|
||
import { DataSorting } from '../../plugins/data-sorting'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious what allows you to declare import
s not at the top of the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the import linting plugin (same as we use internally) starts a new set of groups after a comment
expectTypeOf<OptionsFor<FullFreeformSignature>>().toEqualTypeOf<AOptions>(); | ||
expectTypeOf<TableMetaFor<FullFreeformSignature>>().toEqualTypeOf<ATableMeta>(); | ||
expectTypeOf<ColumnMetaFor<FullFreeformSignature>>().toEqualTypeOf<AColumnMeta>(); | ||
expectTypeOf<RowMetaFor<FullFreeformSignature>>().toEqualTypeOf<ARowMeta>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type testing is quite interesting... Are there any good resources on the philosophy behind them, best practices, etc? Or do we wait for you to give a talk on it? ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, tbh. I learned everything from
- https://github.com/mmkal/expect-type
- the ember discord
- and getting tired of manually testing type inference / intellisense 😅
/** | ||
* @public | ||
* | ||
* utility class to help with autocompletion / documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
new (...args: Args): T; | ||
} | ||
export type Constructor<T, Args extends any[] = any[]> = new (...args: Args) => T; | ||
// export type Constructor<T, Args extends any[] = any[]> = abstract new (...args: Args) => T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented line. Remove? or if not, add comment to explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh! yup, this can be removed
let result = this.plugins.find((plugin) => plugin instanceof klass); | ||
|
||
/** | ||
* This is an unsafe cast, because Instance could be unrelated to any of the types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so... if it's unsafe, can we explain why we are doing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added more explanation! thanks!
Really nice improvements here! So what will the migration path look like for someone upgrading to this version? I'm assuming this'll be a major bump, right? |
@@ -48,14 +48,22 @@ export interface TableOptions { | |||
handlePosition?: string; | |||
} | |||
|
|||
interface Signature { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much nicer to read!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
* When interacting with preferences, the value stored in preferenced | ||
* will be the inverse of this value (to save space in storage). | ||
*/ | ||
isVisible?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this example of custom Column options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Co-authored-by: Dan Wenzel <dan@danwenzel.com>
Co-authored-by: Dan Wenzel <dan@danwenzel.com>
Naw, internally what I've been doing is:
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This was originally implemented here, but was extracted due to being a (required) side-quest of that PR in order to support that PR's original goal.
This PR changes the type arguments for all plugins to use a single type argument, called
Signature
, inline with (Modifiers / Helpers / Components / Resources)' nomenclature.Docs: https://34dd5187.ember-headless-table.pages.dev/api/interfaces/plugins.PluginSignature (hash might change)