Skip to content
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

feat: migrate to TypeScript #107

Merged
merged 2 commits into from
Nov 26, 2020
Merged

feat: migrate to TypeScript #107

merged 2 commits into from
Nov 26, 2020

Conversation

kentcdodds
Copy link
Owner

@kentcdodds kentcdodds commented Nov 25, 2020

What: Migrate to TypeScript!!! 🔥

Why: Because TypeScript is great

How: Lots of clicky-clack on my keyboard

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

BREAKING CHANGE: the API for baseSort arguments now uses rankedValue instead of rankedItem
BREAKING CHANGE: Now using String.prototype.{startsWith,includes}. Older browsers must have a polyfill for these modern APIs.

@codecov
Copy link

codecov bot commented Nov 25, 2020

Codecov Report

Merging #107 (1f2d9b4) into master (7158295) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #107   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          114       137   +23     
  Branches        28        31    +3     
=========================================
+ Hits           114       137   +23     
Impacted Files Coverage Δ
src/index.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7158295...1f2d9b4. Read the comment docs.

@kentcdodds
Copy link
Owner Author

I think I'm going to build-in a few things in kcd-scripts before merging this so I don't have to have that tsconfig.build.json file just to generate the type declarations.

src/index.ts Outdated
@@ -29,26 +61,31 @@ const defaultBaseSortFn = (a, b) =>
* @param {Object} options - Some options to configure the sorter
* @return {Array} - the new sorted array
*/
function matchSorter(items, value, options = {}) {
function matchSorter<ItemType>(
Copy link

Choose a reason for hiding this comment

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

Maybe specify a default type?

src/index.ts Outdated

declare function valueGetterKey<ItemType>(item: ItemType): string
declare function baseSortFn<ItemType>(
a: Merge<RankingInfo, {item: ItemType; index: number}>,
Copy link
Contributor

Choose a reason for hiding this comment

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

with interfaces (or even just &, but the interface API is cleaner IMO), you can get rid of this dependency:

interface RankingInfo {
  rankedValue: string
  rank: number
  keyIndex: number
  keyThreshold: number | undefined
}
interface IndexedItem<ItemType> {
  item: ItemType
  index: number
}
interface RankedItem<ItemType> extends RankingInfo, IndexedItem<ItemType> {}

declare function baseSortFn<ItemType>(
  a: RankedItem<ItemType>,
  b: RankedItem<ItemType>,
): number

src/index.ts Outdated
keyThreshold: number | undefined
}

declare function valueGetterKey<ItemType>(item: ItemType): string
Copy link
Contributor

@rbusquet rbusquet Nov 25, 2020

Choose a reason for hiding this comment

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

for the declare function, I'd also prefer going with an interface. Unfortunately(? or maybe it's fine), this will cause a couple of other types you're exporting to be generics (but it's not terrible);

interface ValueGetterKey<ItemType> {
  (item: ItemType): string
}

interface BaseSorter<ItemType> {
  (a: RankedItem<ItemType>, b: RankedItem<ItemType>): number
}

interface KeyAttributesOptions<ItemType> {
  key?: string | ValueGetterKey<ItemType>
  threshold?: number
  maxRanking?: number
  minRanking?: number
}

type KeyOption<ItemType> =
  | KeyAttributesOptions<ItemType>
  | ValueGetterKey<ItemType>
  | string

// ItemType = unknown allowed me to make these changes without the need to change the current tests
interface MatchSorterOptions<ItemType = unknown> {
  keys?: Array<KeyOption<ItemType>>
  threshold?: number
  baseSort?: BaseSorter<ItemType>
  keepDiacritics?: boolean
}
// ...
function matchSorter<ItemType>(
  items: Array<ItemType>,
  value: string,
  options: MatchSorterOptions<ItemType> = {}, // I make sure to pass `ItemType` to the type I defaulted to unknown to get proper type inference
): Array<ItemType> {

@rbusquet
Copy link
Contributor

here's the full patch I worked on https://gist.github.com/rbusquet/17a593712ebcc9ea973b18d0dfeea034

@jensmeindertsma
Copy link

I'd suggest making sure types are imported separately using the import type syntax 👍.

@marcosvega91
Copy link
Contributor

💪 you are great !!

@kentcdodds
Copy link
Owner Author

I'm trying to figure out how to make this work for my UMD build (globals). Here's the generated type defs (I haven't applied the suggested changes yet):

import type { Merge } from 'type-fest';
declare type KeyAttributes = {
    threshold?: number;
    maxRanking: number;
    minRanking: number;
};
declare type RankingInfo = {
    rankedValue: string;
    rank: number;
    keyIndex: number;
    keyThreshold: number | undefined;
};
declare function valueGetterKey<ItemType>(item: ItemType): string;
declare function baseSortFn<ItemType>(a: Merge<RankingInfo, {
    item: ItemType;
    index: number;
}>, b: Merge<RankingInfo, {
    item: ItemType;
    index: number;
}>): number;
declare type KeyAttributesOptions = {
    key?: string | typeof valueGetterKey;
    threshold?: number;
    maxRanking?: number;
    minRanking?: number;
};
declare type KeyOption = KeyAttributesOptions | typeof valueGetterKey | string;
declare type MatchSorterOptions = {
    keys?: Array<KeyOption>;
    threshold?: number;
    baseSort?: typeof baseSortFn;
    keepDiacritics?: boolean;
};
declare const rankings: {
    CASE_SENSITIVE_EQUAL: number;
    EQUAL: number;
    STARTS_WITH: number;
    WORD_STARTS_WITH: number;
    CONTAINS: number;
    ACRONYM: number;
    MATCHES: number;
    NO_MATCH: number;
};
/**
 * Takes an array of items and a value and returns a new array with the items that match the given value
 * @param {Array} items - the items to sort
 * @param {String} value - the value to use for ranking
 * @param {Object} options - Some options to configure the sorter
 * @return {Array} - the new sorted array
 */
declare function matchSorter<ItemType>(items: Array<ItemType>, value: string, options?: MatchSorterOptions): Array<ItemType>;
declare namespace matchSorter {
    var rankings: {
        CASE_SENSITIVE_EQUAL: number;
        EQUAL: number;
        STARTS_WITH: number;
        WORD_STARTS_WITH: number;
        CONTAINS: number;
        ACRONYM: number;
        MATCHES: number;
        NO_MATCH: number;
    };
}
export { matchSorter, rankings, MatchSorterOptions, KeyAttributesOptions, KeyOption, KeyAttributes, RankingInfo, baseSortFn, valueGetterKey, };

How do I expose those to the global namespace? I tried following these instructions: https://mariusschulz.com/blog/declaring-global-variables-in-typescript But couldn't figure out why it wasn't working 🤔

@rbusquet
Copy link
Contributor

@kentcdodds I don't know how it helps but I get TS to recognize types correctly if I add "types": "./dist/index.d.ts", to my package.json. Have you tried that?

@kentcdodds
Copy link
Owner Author

Yeah, that works. Though, if someone wants to import one of the bundled files that will not work, so I think for projects that use the --bundle flag for kcd-scripts build, I'll duplicate and rename that file to be a .d.ts for each of the built/bundled files so it "just works" even if they import a bundled file directly.

Also, I think I'll skip the UMD build for this.

From here, I think I know what changes need to be made in kcd-scripts to support this, so I'll do that and upgrade it here before continuing. Thanks!

@kentcdodds
Copy link
Owner Author

I think this will work very well: kentcdodds/kcd-scripts#176

Feedback welcome :)

@kentcdodds
Copy link
Owner Author

kentcdodds commented Nov 26, 2020

I figured out the best way to solve the bundling issue by simply generating the type defs, and then creating a match-sorter.cjs.d.ts, match-sorter.esm.d.ts, etc... that simply contains:

export * from ".";

Which will simply re-export the types in index.d.ts. Everything will just work™️

@kentcdodds
Copy link
Owner Author

@all-contributors please add @rbusquet for ideas and review

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @rbusquet! 🎉

@kentcdodds
Copy link
Owner Author

@all-contributors please add @weyert for ideas and review

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @weyert! 🎉

@kentcdodds
Copy link
Owner Author

@all-contributors please add @MichaelDeBoey for review

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @MichaelDeBoey! 🎉

BREAKING CHANGE: the API for baseSort arguments now uses rankedValue instead of rankedItem
BREAKING CHANGE: Now using String.prototype.{startsWith,includes}. Older browsers must have a polyfill for these modern APIs.
@kentcdodds kentcdodds merged commit ef8a106 into master Nov 26, 2020
@kentcdodds kentcdodds deleted the pr/TYPESCRIPT branch November 26, 2020 15:27
@github-actions
Copy link

🎉 This PR is included in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants