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

feat: add multi-instance support #449

Merged

Conversation

elias-pap
Copy link
Contributor

closes #255

@elias-pap elias-pap requested a review from edno as a code owner February 26, 2022 17:33
@ghost
Copy link

ghost commented Feb 26, 2022

CodeSee Review Map:

Review these changes using an interactive CodeSee Map

Review in an interactive map

View more CodeSee Maps

Legend

CodeSee Map Legend

@edno
Copy link
Member

edno commented Feb 28, 2022

Thank you @elias-pap for this new feature!!! That's awesome.

I will deep dive into the code asap.

In the meanwhile, could you add some tests that cover the multi instance case?

@elias-pap
Copy link
Contributor Author

elias-pap commented Feb 28, 2022

Thank you @elias-pap for this new feature!!! That's awesome.

I will deep dive into the code asap.

In the meanwhile, could you add some tests that cover the multi instance case?

Thanks for this useful plugin @edno !
I added a basic test, let me know what you think.

Copy link
Member

@edno edno left a comment

Choose a reason for hiding this comment

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

I finished the 1st review.

A few comments, but nothing that requires major changes.

Great job @elias-pap 💪

src/index.js Outdated Show resolved Hide resolved
tests/__data__/tweet-B.graphql Outdated Show resolved Hide resolved
tests/__data__/tweet-A.graphql Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
Earthfile Outdated Show resolved Hide resolved
Earthfile Outdated Show resolved Hide resolved
@edno
Copy link
Member

edno commented Mar 3, 2022

I am wondering if this feature should be closer to the current Docusaurus multi-instance approach, see https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-plugin-content-docs/src/index.ts#L71

This would mean one command per instance (id), instead of one command for all. That would give a more granular control over the generation to users, and users can still merge both command into a single one with a script.

@elias-pap
Copy link
Contributor Author

elias-pap commented Mar 3, 2022

I am wondering if this feature should be closer to the current Docusaurus multi-instance approach, see https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-plugin-content-docs/src/index.ts#L71

This would mean one command per instance (id), instead of one command for all. That would give a more granular control over the generation to users, and users can still merge both command into a single one with a script.

That's right, this is how I had implemented it initially and the change required is far more simpler:

const command = opts.id === "default" ? "graphql-to-doc" : `graphql-to-doc:${opts.id}`;
.
.
.
return {
    name: "docusaurus-graphql-doc-generator",

    extendCli(cli) {
      cli
        .command("graphql-to-doc")  // replace with .command(command) here
        .option("-s, --schema <schema>", "Schema location")

"default" is the value of DEFAULT_PLUGIN_ID.

I changed my mind later on because this mutates the name of the command, and this seemed a bit weird. Docusaurus handles that detail internally for their docs plugin, so the user does not have to deal with many commands.

But yeah, we can do this instead ! Whatever you think is best.

@edno
Copy link
Member

edno commented Mar 5, 2022

I changed my mind later on because this mutates the name of the command, and this seemed a bit weird. Docusaurus handles that detail internally for their docs plugin, so the user does not have to deal with many commands.

Docusaurus also changes the command name (and description): https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-plugin-content-docs/src/index.ts#L89

Both approaches are valid. I would like to stay as close as possible to Docusausus behaviour for the v1 (the v2 will one day allow breaking the dependency to Docusaurus).

Ideally, I'd love to see the default command running all. But that will be a nightmare to solve this. And, to stick to KISS principle (I should add this to the contrib guidelines), I think we should go with Docusaurus command per id.

const isDefaultId = opts.id === "default";

const command = isDefaultId ? "graphql-to-doc" : `graphql-to-doc:${opts.id}`;
const description = `Generate GraphQL Schema Documentation for ${opts.schema}`; // trick for helping users

return {
    name: "docusaurus-graphql-doc-generator",

    extendCli(cli) {
      cli
        .command(command)
        .description(description)   

And, we can keep the config.js logic for merging config with cli option. I like that approach, it makes the code cleaner.

@elias-pap elias-pap requested a review from edno March 6, 2022 00:19
src/index.js Show resolved Hide resolved
edno
edno previously approved these changes Mar 6, 2022
Earthfile Outdated Show resolved Hide resolved
@elias-pap elias-pap requested a review from edno March 7, 2022 10:41
@sonarcloud
Copy link

sonarcloud bot commented Mar 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@edno edno enabled auto-merge (squash) March 7, 2022 20:30
@edno edno disabled auto-merge March 7, 2022 20:30
@edno edno merged commit 1cb0d67 into graphql-markdown:main Mar 7, 2022
@elias-pap elias-pap deleted the feature/add-multi-instance-support branch March 13, 2022 12:56
@edno
Copy link
Member

edno commented Mar 13, 2022

Released: https://github.com/edno/graphql-markdown/releases/tag/1.9.0 🎉

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

Successfully merging this pull request may close these issues.

Allow for more than one instance of graphql-doc-generator in docusaurus.config
2 participants