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

Exposing useful types in Typesense.ts #116

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

reinoldus
Copy link

Change Summary

  • Exposing useful types in Typesense.ts
  • Adding ts-eslint
  • Adding es-link option to use import type (prevents types to end up in bundle)

These are my suggestions for the things we discussed in slack. The tests are currently failing because of ts-eslint and all the typescript errors.

What are your thoughts?

PR Checklist

- Adding ts-eslint
- Adding es-link option to use import type (prevents types to end up in bundle)
@reinoldus
Copy link
Author

reinoldus commented Mar 24, 2022

For context please follow this slack conversation: https://typesense-community.slack.com/archives/C01P749MET0/p1647516018829819

Regarding the namespace I was thinking about the same but then I came across this part of the manual: https://www.typescriptlang.org/docs/handbook/modules.html#export-as-close-to-top-level-as-possible

It probably wouldn't make sense to group them all under "types" or something but splitting them up into logical units (Documents, etc.) is probably to much.

I also auto-completion in webstorm can't handle it properly, if I try to auto-import this type: RequestUnauthorized from Errors, it imports it from the lib-path and not from "typesene".

Also the user always has the option to do the following:

import type * as t from 'typesense'

Regarding the unchanged files popping up in the PR: I don't know, I haven't changed any modes of any file etc.. I just ran eslint --fix . on enverything. Could there be an eslint rule that forces some kind of mode onto the files?
What OS are you running? It could also be win <-> lin (I am on ubuntu) conversion. Even though I would have assumed github would sort this out by default.

EDIT:

Regarding the constructor error messages: I disabled that rule. Eslint is quite opinionated

EDIT2:

Yes it was a line ending issue:
Before files were kind of a colorful mix:

i/none  w/none  attr/                   lib/Typesense/SearchClient.js.map
i/lf    w/lf    attr/                   lib/Typesense/SearchOnlyCollection.d.ts
i/lf    w/lf    attr/                   lib/Typesense/SearchOnlyCollection.js
i/none  w/none  attr/                   lib/Typesense/SearchOnlyCollection.js.map
i/lf    w/lf    attr/                   lib/Typesense/SearchOnlyDocuments.d.ts
i/lf    w/lf    attr/                   lib/Typesense/SearchOnlyDocuments.js
i/none  w/none  attr/                   lib/Typesense/SearchOnlyDocuments.js.map
i/lf    w/lf    attr/                   lib/Typesense/Synonym.d.ts
i/lf    w/lf    attr/                   lib/Typesense/Synonym.js
i/none  w/none  attr/                   lib/Typesense/Synonym.js.map
i/lf    w/lf    attr/                   lib/Typesense/Synonyms.d.ts
i/lf    w/lf    attr/                   lib/Typesense/Synonyms.js
i/none  w/none  attr/                   lib/Typesense/Synonyms.js.map
i/none  w/none  attr/                   mocha.opts
i/lf    w/lf    attr/                   package-lock.json
i/lf    w/lf    attr/                   package.json
i/-text w/-text attr/                   src/.DS_Store
i/crlf  w/crlf  attr/                   src/Typesense.ts
i/mixed w/mixed attr/                   src/Typesense/Alias.ts
i/crlf  w/crlf  attr/                   src/Typesense/Aliases.ts
i/mixed w/mixed attr/                   src/Typesense/ApiCall.ts
i/mixed w/mixed attr/                   src/Typesense/Client.ts
i/mixed w/mixed attr/                   src/Typesense/Collection.ts
i/crlf  w/crlf  attr/                   src/Typesense/Collections.ts

Unknowingly I converted them all to LF. You can check using this option from git:
git ls-files --eol

Here is some document on how to prevent that::
https://docs.github.com/en/get-started/getting-started-with-git/configuring-git-to-handle-line-endings

@jasonbosco
Copy link
Member

It probably wouldn't make sense to group them all under "types" or something but splitting them up into logical units (Documents, etc.) is probably to much.

Sounds good.

Regarding the constructor error messages: I disabled that rule. Eslint is quite opinionated

👍

re: line-ending issue, good to know! I'm on on a mac, and I think some past contributors were using Windows. For the purposes of keeping this PR clean and easy to parse the true diff, mind disabling and reverting the line endings change? I'll take care of standardizing it separately.

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

Successfully merging this pull request may close these issues.

2 participants