search

adopted-ember-addons / ember-sortable

Sortable UI primitives for Ember.js
https://ember-sortable.netlify.app/
MIT License
298 stars 148 forks source link

Add TypeScript type definitions #565

Closed charlesfries closed 4 weeks ago

charlesfries commented 3 months ago

Should fix #564

NullVoxPopuli commented 3 months ago

any chance you can add type-checking to CI within the test-app?

the test-app could just have one TS file that is type-checked -- no need to convert the whole test-app to TS

charlesfries commented 3 months ago

@NullVoxPopuli One point of clarification--do you want me to import these definitions I've added in this PR into that TS file, or just convert some existing file to TS?

Also, I'm trying to figure out which file to convert to TS. Most files in test-app import Ember modules, which necessitates upgrading ember-source to a later version that includes the built in TS types. I can definitely do that, but I'm wondering if that should be a different PR entirely. Another option is just installing one or two of the DefinitelyTyped packages.

NullVoxPopuli commented 3 months ago

it can be a standalone file -- no need to convert existing stuff.

like, you could have app/type-tests in the test-app, and have scenarios specifically for ensuring the APIs are correct -- no need to have runnable code as glint would verify that the types are correct.

, which necessitates upgrading ember-source to a later version that includes the built in TS types.

we could also do a separate project, maybe test-types (rather than test-app), which is the most minimal set of files we need to type check -- could even use built in types, and you don't need most of the files that come with an app

netlify[bot] commented 3 months ago

Deploy Preview for ember-sortable canceled.

Name Link
Latest commit 45da1dc0f5696117f6b32eeccf8c98a227297446
Latest deploy log https://app.netlify.com/sites/ember-sortable/deploys/66c4cc0ab168d1000814e9a8
mkszepp commented 4 weeks ago

converted the complete addon to TS #581 so this PR is not anymore needed... btw... the signatures here are not complety