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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: elaborate Repository.execute method #6453
Conversation
Add more details to the Repository.execute API doc. Signed-off-by: Yaapa Hage <hage.yaapa@in.ibm.com>
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.
Please note the support for non-SQL connectors has been already landed. #3342 is mostly done, the remaining work is to investigate one test failure for PostgreSQL and update connector docs.
A detailed documentation for different execute
flavors was added in #6030. The api doc comments are in DefaultCrudRepository
, because I feel the content is specific to the implementation provided by loopback-datasource-juggler and our connectors.
See also loopbackio/loopback-datasource-juggler#1855.
There is also "regular" documentation in Executing database commands, it was added by #6047.
In the light of the above, your proposal seems to me as out of touch with the current status. At the same time, I see how the current ExecutableRepository
API and docs are not consistent with the rest of the framework, so let's discuss how to get it on par.
The repo method is indeed ready for both SQL and no-SQL, that note is meant for the feature support in our official connectors. I have tested with MySQL connector it works fine. Looked at the Postgres connector, it should work. Looked at the MongoDB connector, it doesn't look like it would work; and when tried didn't work as expected. When we don't have the MongoDB connector working, how do "Add a MongoDB example as part of this story."? Or did I misunderstand something about the MongoDB connector? |
Oh, I got the MongoDB connector working. It was a case of mismatched expectation. We should document that the result object will be whatever the underlying connector returns, and that Repository will not perform any serialization or optimization. |
Feedback Signed-off-by: Yaapa Hage <hage.yaapa@in.ibm.com>
@bajtos I have updated the PR. |
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 feel this pull request is making the situation a bit messy. It does add more information to ExecutableRepository.execute
, but this information is partially duplicating documentation from DefaultCrudRepository.execute
and does not cover everything documented in DefaultCrudRepository
.
I'd like us to step back, ask ourselves what is the goal of #5264 and how to reach it, considering all the works that has been already done in #6034.
I opened #5264 because it was not clear to me how to use execute
method and found it difficult to help our users asking questions on StackOverflow without that information. #6034 added the information needed, but it may be difficult to find when looking at ExecutableRepository.execute
docs.
The first question we should ask: How much juggler/connector-specific information do we want to encode into ExecutableRepository.execute
? I see two options:
- Keep
ExecutableRepository.execute
generic, document connector-specific stuff inDefaultCrudRepository
. - Push connector-specific things (method overloads, API doc comments) to
ExecutableRepository.execute
.
If we pick the first option, then I thin we should change the signature of ExecutableRepository.execute
to (...args: PositionalParameters)
, to make sure it covers all different connector flavors. Documentation wise, we can add links (e.g. using @see
keyword) to different DefaultCrudRepository.execute
flavors (SQL, MongoDB, generic). No need to duplicate the content.
If we pick the second option, then I would move most (if not all) of API docs from DefaultCrudRepository
to ExecutableRepository
, together with the different method overloads (SQL, MongoDB, generic). I don't know our API docs tooling will provide a link from DefaultCrudRepository.execute
api docs to the method we are inheriting from (ExecutableRepository.execute
). If it does not, then we should add a link from DefaultCrudRepository.execute
to ExecutableRepository.execute
.
To re-iterate: we already have the content written, now we need a better way how to organize it and make it easier to find, while avoiding duplication. Feel free to choose a different approach as long as it provides a good DX and meets those high-level goals.
* | ||
* const res = await myRepository.execute( | ||
* 'Pet', 'find', {type:'cat'} | ||
* ); |
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.
Are you sure the string 'find'
is assignable to NamedParameters | PositionalParameters
?
ATM, ExecutableRepository.execute
provides SQL-like flavor only:
interface ExecutableRepository {
execute(
command: Command,
parameters: NamedParameters | PositionalParameters,
options?: Options,
): Promise<AnyObject>;
}
To support other connectors, I had to add few more overloads in #6030:
interface ExecutableRepository {
// MongoDB
execute(
collectionName: string,
command: string,
...parameters: PositionalParameters
): Promise<AnyObject>;
// Other connectors
execute(...args: PositionalParameters): Promise<AnyObject>;
}
+1. IMO it would be good if we can somehow navigate to |
Most of the original goal of this PR is already in place now, better to open a new issue and PR as and when required. Closing this. |
Addresses #5264. Adds more details to the Repository.execute API doc.
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated馃憠 Check out how to submit a PR 馃憟