-
Notifications
You must be signed in to change notification settings - Fork 74
feat: Shell API autocomplete type definitions MONGOSH-2031 MONGOSH-2032 MONGOSH-2173 #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
Conversation
```sh cd packages/shell-api && \ npm run compile && \ npx api-extractor run ; \ npx ts-node bin/api-postprocess.ts ; \ cat lib/api-processed.d.ts ```
packages/shell-api/package.json
Outdated
"types": "./lib/index.d.ts" | ||
}, | ||
"./api": { | ||
"types": "./lib/api-processed.d.ts" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end I have to fs.readFile() to get the file as text so we can bundle it as a giant string, so this is kinda moot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end I have to fs.readFile() to get the file as text so we can bundle it as a giant string, so this is kinda moot.
Yeah, exactly. Do we do this as part of this PR? Put it in a .ts
file that only exports that giant string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now I generate that giant string in mongodb-ts-autocomplete (see npm run extract-types in mongodb-js/devtools-shared#520), but I suppose we can also export it here. I don't know which way is best. I don't want to accidentally bundle a giant string..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ... the two options seem to be:
- Make shell-api a dev dependency of mongodb-ts-autocomplete, run the extract-types script there as a compile/prepublish step and then include the generated "big string" file in the mongodb-ts-autocomplete exports
- Make shell-api a peer dependency of mongodb-ts-autocomplete, move the "big string" generation script here a compile/prepublish and export it from shell-api, then consume it from mongodb-ts-autocomplete
Obviously both of these work, but I don't really see disadvantages to the latter; the former does have at least one notable disadvantage (having to update mongodb-ts-autocomplete to incorporate shell-api changes).
I don't want to accidentally bundle a giant string..
I think we'll end up bundling this string in all our products anyway. But for now, this should actually just work fine, right? webpack wouldn't include it if it isn't being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I do now: 0bd794f
Then in devtools-shared I can:
import ShellApiText from '@mongosh/shell-api/api';
Without having to require.resolve() and read the file and all that. Which is.. marginally better, I guess? I still read mql lib for now (until we do the same thing in the mql types package) and I still do it for reading the bson package.
We can only really get rid of the bson package if we revert 9b3779d and therefore bundle the bson types we use inside the extracted API. But then we'd have to do the same in mql types too.
'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()}` |
There was a problem hiding this comment.
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.
"@mongosh/service-provider-core", | ||
"@mongosh/types", | ||
"mongodb", | ||
"bson" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have bson set up in mongodb-ts-autocomplete, so I might remove it from here. We'd then just have to find/replace the line where we import from 'bson' to import from '/bson.ts' so that it will be loaded from the typescript service at runtime. (I already got that working for my stubbed mql and shell api implementations, so we can just reuse it.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it from here and then extract-types.ts in mongodb-ts-autocomplete in devtools-shared replaces the bson import to use its one. That way we can use the same bson shared everywhere (shell-api and mql) and we don't end up extracting all the types again and again. This might premature optimisation, I don't know. But it isn't hard to undo either.
@@ -0,0 +1,44 @@ | |||
declare global { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is basically a snippet that gets appended at the end of api-globals.d.ts after we api-extract that. See bin/api-postprocess.ts.
I first tried to write a babel plugin to walk the ShellAPI class's methods and the ShellBson interface's methods, but that turned out to be really complicated.
There is one benefit to having them all mapped here explicitly: We don't end up accidentally leaking everything on ShellAPI into the global scope whether we want to or not. And it is arguably handy seeing them all enumerated here.
The only other variables added as globals would be db, sh, sp and rs which all get specified dynamically by mongodb-ts-autocomplete so they reference the server/database/collection schema generics.
packages/shell-api/package.json
Outdated
"types": "./lib/index.d.ts" | ||
}, | ||
"./api": { | ||
"types": "./lib/api-processed.d.ts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end I have to fs.readFile() to get the file as text so we can bundle it as a giant string, so this is kinda moot.
Yeah, exactly. Do we do this as part of this PR? Put it in a .ts
file that only exports that giant string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented on the giant-string location question but either way, LGTM
packages/shell-api/package.json
Outdated
"types": "./lib/index.d.ts" | ||
}, | ||
"./api": { | ||
"types": "./lib/api-processed.d.ts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ... the two options seem to be:
- Make shell-api a dev dependency of mongodb-ts-autocomplete, run the extract-types script there as a compile/prepublish step and then include the generated "big string" file in the mongodb-ts-autocomplete exports
- Make shell-api a peer dependency of mongodb-ts-autocomplete, move the "big string" generation script here a compile/prepublish and export it from shell-api, then consume it from mongodb-ts-autocomplete
Obviously both of these work, but I don't really see disadvantages to the latter; the former does have at least one notable disadvantage (having to update mongodb-ts-autocomplete to incorporate shell-api changes).
I don't want to accidentally bundle a giant string..
I think we'll end up bundling this string in all our products anyway. But for now, this should actually just work fine, right? webpack wouldn't include it if it isn't being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - I mostly have naming/nitty suggestions, but the core logic makes sense to me.
M extends GenericServerSideSchema = GenericServerSideSchema, | ||
D extends GenericDatabaseSchema = GenericDatabaseSchema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm too old or perhaps due to not being a ts developer, but these are overly vague for me. Fine leaving them as they are if that's idiomatic for ts, but any reason not to use more descriptive generic names here? Reading those further down in the file, it's very much unclear what M
or D
is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admittedly just left them as I found them and haven't given it much thought. Any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt like the default values make it clear enough what these are respectively, and it doesn't take a ton of getting used to, but if there are better names for these, then I don't think there's any reason not to adopt them
packages/shell-api/src/bulk.spec.ts
Outdated
@@ -57,7 +57,7 @@ describe('Bulk API', function () { | |||
}); | |||
describe('Metadata', function () { | |||
describe('toShellResult', function () { | |||
const collection = stubInterface<Collection>(); | |||
const collection = stubInterface<CollectionImpl>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to be stubbing CollectionImpl here (and elsewhere instead of Collection)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collection (like Database) is now just a type, not a class. This is what I called out in a review comment here. We can keep Collection and Database as classes and then make types named something like CollectionWithSchema and DatabaseWithSchema.
I'm in two minds about which way around is better. I just left it this way because it is what @addaleax did in the PoC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose there is probably also a third way where we just extend the class and end up with another Class rather than building a type 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CollectionImpl is now gone and unless something is a unit test that just checks Collection, CollectionWithSchema is the thing that's used.
@@ -131,7 +131,7 @@ describe('getPrintableShardStatus', function () { | |||
const testServer = startSharedTestServer(); | |||
|
|||
let mongo: Mongo; | |||
let configDatabase: Database; | |||
let configDatabase: DatabaseImpl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not Database
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all fallout from the Database vs DatabaseImpl and Collection vs CollectionImpl thing above. But also as it stands Database & Collection are now generic which means every unit test in the codebase that uses those have to specify the generic types which.. would be a very large PR. And unit tests generally don't care about the schema provided by the generics - they just test the base Collection and Database classes which are just called DatabaseImpl and CollectionImpl now.
I think I can probably "solve" it by renaming CollectionImpl back to Collection and DatabaseImpl back to Database then making either derived classes or types called CollectionWithSchema and DatabaseWithSchema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DatabaseImpl is gone now. I also removed the default exports from collection.ts and database.ts to force you to pick something explicitly.
DatabaseWithSchema and CollectionWithSchema are the ones you want if you want types, Database and Collection are only things you need when constructing objects, but that's rarely done in the main code and also rare in unit tests where it gets done when unit testing the database or collection and I think that's fair. Can nitpick over whether some existing tests should be making their own database or collection objects in the first place, but I'd prefer leaving them to do what they are already doing in this PR to keep the change size down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM – only real blocker is the fact that we need to make sure to return DatabaseWithSchema
and CollectionWithSchema
in public APIs now
expect(output).not.to.include('Tab completion error'); | ||
expect(output).not.to.include( | ||
expect(output, output).not.to.include('Tab completion error'); | ||
expect(output, output).not.to.include( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because the output was otherwise too short?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't remember the exact way in which it was useless :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kinda a hacky but very easy way to make sure it print the full output if it ever fails which is the very first thing anyone debugging it would do. And it should keep quiet when the expect succeeds so is kinda zero cost.
M extends GenericServerSideSchema = GenericServerSideSchema, | ||
D extends GenericDatabaseSchema = GenericDatabaseSchema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt like the default values make it clear enough what these are respectively, and it doesn't take a ton of getting used to, but if there are better names for these, then I don't think there's any reason not to adopt them
Co-authored-by: Anna Henningsen <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
…om/mongodb-js/mongosh into shell-api-ac-type-definitions-fork
This is a continuation of Anna's work here.
Usage:
Implements:
Changes:
@mongosh/shell-api/api
which is just api-processed exported as one giant string.Notes: