olivernn / lunr.js

A bit like Solr, but much smaller and not as bright
http://lunrjs.com
MIT License
8.91k stars 546 forks source link

Mixing definition of const/enum within string makes it hard to write typescript definition #297

Closed seantanly closed 7 years ago

seantanly commented 7 years ago

https://github.com/olivernn/lunr.js/blob/81b4fd6547ce3155820406b09ae417a4c10431ed/lib/query.js#L40-L43

The clever use of defining enum constants within the instance of string "*" makes it hard to write type definition for typescript.

May I suggest adding lunr.Query.wildcardSymbol = "*"

 lunr.Query.wildcardSymbol = "*"
 lunr.Query.wildcard = new String (lunr.Query.wildcardSymbol) // Deprecated
 lunr.Query.wildcard.NONE = 0 
 lunr.Query.wildcard.LEADING = 1 
 lunr.Query.wildcard.TRAILING = 2 

This allows the typescript definition to ignore lunr.Query.wildcard as a string, and prevent breakage to existing users who rely on lunr.Query.wildcardas "*".

 namespace lunr {
   namespace Query {
    const wildcardSymbol: "*"

    enum wildcard {
      NONE = 0,
      LEADING = 1 << 0,
      TRAILING = 1 << 1,
    }
 }
olivernn commented 7 years ago

+1 for the backwards compatible version, but does it need to be part of the TypeScript interface? The enum values do, but I don't think the string lunr.Query.wildcard is really used as part of the public API, its not even used in other places within the JavaScript implementation (which could/should probably be fixed). I don't know what the best practise is when defining interfaces/typings for TypeScript though.

seantanly commented 7 years ago

I have considered lunr.Query.wildcard because it seems to be part of the public interface - placed after comments. It can be excluded from the TypeScript interface.

Searching around, lunr.Query.wildcard is used only at

https://github.com/olivernn/lunr.js/blob/81b4fd6547ce3155820406b09ae417a4c10431ed/lib/query.js#L84 https://github.com/olivernn/lunr.js/blob/81b4fd6547ce3155820406b09ae417a4c10431ed/lib/query.js#L88

which could easily be replaced just with "*" and then lunr.Query.wildcard can be removed.

seantanly commented 7 years ago

As suggested, I left out lunr.Query.wildcard in the typescript definition.