Skip to content

feat: Shell API autocomplete type definitions MONGOSH-2032 #2434

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

lerouxb
Copy link
Contributor

@lerouxb lerouxb commented Apr 17, 2025

This is a continuation of Anna's work here.

  • merged in main, resolving the conflicts
  • added generics to the rs, sh and sp objects
  • added a separate api.ts entrypoint, generating api-processed.d.ts (although this needs work - trying to figure out how to reference it in package.json)
  • bundled more deps so that we don't import those types in the api-processed.d.ts file. The node.js packages are still bing imported, though. Will have to test if that works with the language server
  • fixed the tests (at least the ones I've noticed were broken so far)
  • Split out MONGOSH-2171, MONGOSH-2172, MONGOSH-2173 into separate tickets.

Notes:

@@ -294,7 +294,7 @@ describe('completer.completer', function () {

it('returns all suggestions', async function () {
const i = 'db.';
const attr = shellSignatures.Database.attributes as any;
const attr = shellSignatures.DatabaseImpl.attributes as any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fact that Database and Collection are now types and the classes got replaced with DatabaseImpl and CollectionImpl is having a lot of knock-on effects and kinda ended up being responsible for most of this diff.

I'm wondering if we should keep them as Database and Collection but then rather name the new types to be something like DatabaseWithSchema and CollectionWithSchema. Then maybe the change will be smaller?

Not sure which way around would work out better ergonomically overall.

"types": "./lib/index.d.ts"
},
"./api": {
"types": "./lib/api-processed.d.ts"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what would be the best way to export/publish this and then reference it from the outside.

I think import type ShellApi from '@monogdb-js/shell-api/api' works with this and I tried it manually from a file, but I'll see shortly when I plug this into mongodb-ts-autocomplete

InterruptFlag,
};

// TODO: do we really want all these?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a reminder for myself to remove this TODO comment before merging and after bringing it up with the team.

it('calls help function', async function () {
expect((await toShellResult(apiClass.help())).type).to.equal('Help');
expect((await toShellResult(apiClass.help)).type).to.equal('Help');
});
});
describe('signatures', function () {
it('type', function () {
expect(signatures.Collection.type).to.equal('Collection');
// TODO: do we want the signatures to be CollectionImpl or Collection?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a reminder for myself to remove this TODO comment before merging and after bringing it up with the team.

@@ -0,0 +1,102 @@
// TODO: does it make sense to have all this stuff? Don't we just need enough for the top-level API, ie. the globals?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a reminder for myself to remove this TODO comment before merging and after bringing it up with the team.

'Running "' + joinedCommand + '" in ' + args[2]?.cwd ?? process.cwd()
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore TS2869 Right operand of ?? is unreachable because the left operand is never nullish.
`Running "${joinedCommand}" in ${args[2]?.cwd ?? process.cwd()}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

drive-by. For some reason I got a compile error on this line in CI.

@lerouxb lerouxb marked this pull request as ready for review April 22, 2025 12:54
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