feat: add mongodb-ts-autocomplete package MONGOSH-2034 #520
Conversation
lerouxb
force-pushed
the
add-autocomplete-package
branch
from
24c2ad2 to
8b624a4
Compare
| position = code.length; | ||
| } | ||
|
|
||
| // TODO: we're now compiling the source twice: autocomplete will also |
There was a problem hiding this comment.
This kind of thing is IMHO an argument for just merging the ts-autocomplete package into this one. Not insurmountable, but it becomes a non-issue if it is just one.
There was a problem hiding this comment.
(but this would also totally be a micro-optimisation at this stage)
There was a problem hiding this comment.
Again - I'll just remove the TODO before merging. Just thought I'd point this out to reviewers. I think this is OK since it is just a line or small multi-line snippet of code and triggering autocomplete is not exactly something that happens in a tight loop. We can optimise it later if we ever have to.
Just wanted to point out that I did think about it 😆
| ); | ||
| activeConnection.setDatabaseCollectionNames(databaseName, collectionNames); | ||
|
|
||
| // TODO: the problem with doing it this way is that, while the collection |
There was a problem hiding this comment.
I think this is probably fine. Getting the sample docs and calculating a schema is what's relatively expensive, probably not walking it and writing a few lines of typescript. Only way this becomes an issue is if you have a really large collection schema or very many databases/collections in the same server schema. Each individual database's list of collections is lazy loaded, though and also each collection's schema. So you'd likely have to have quite the long lived and meandering session for that to build up.
We would likely load the list of databases or collections for at least one database early on, though.
There was a problem hiding this comment.
Similar to comments above, I think I'll remove this comment before merging. Just wanted to point out that I'm aware.
| // names from a listCollections() or it could just be all the ones we've | ||
| // auto-completed for. The schemas can come from the autocompletion context. | ||
| // This can probably be added to the autocompletion context. | ||
| this.collectionNames = new Set(); |
There was a problem hiding this comment.
Thinking about whether DatabaseSchema and ConnectionSchema can just be merged into the autocompletion context and then it keeps track of the known database names and for each database the known collection names.
lerouxb
force-pushed
the
add-autocomplete-package
branch
from
185e497 to
6d9a9d7
Compare
| private readonly databaseSchemas: Record<string, DatabaseSchema>; | ||
|
|
||
| constructor() { | ||
| // TODO: this is kinda the only real reason for this class: So we can keep |
There was a problem hiding this comment.
I'll remove the TODO before merging, just keen on hearing people's thoughts first.
| private collectionSchemas: Record<string, JSONSchema | undefined>; | ||
|
|
||
| constructor() { | ||
| // TODO: this is kinda the only real reason for this class: So we can keep |
There was a problem hiding this comment.
I'll remove the TODO before merging, just keen on hearing people's thoughts first.
|
|
||
| dbRef: new DBRef('namespace', new ObjectId('642d76b4b7ebfab15d3c4a78')), // not actually a separate type, just a convention | ||
|
|
||
| // TODO: what about arrays of objects or arrays of arrays or heterogynous types in general |
There was a problem hiding this comment.
I can ticket this, but it is the kind of thing that we'll probably never do. It doesn't break in those cases: At worst you just get you just get foo: any. It might start to matter more if we use this in vscode or compass, but it can be for that project to enhance this.
lerouxb
changed the title
Currently still works with fake shell api and bson expression types, but the bson types are at least the real ones.
TODO: separate tickets
TODO: this PR