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

Add `GraphClientFactory` #410

Merged
merged 1 commit into from Dec 7, 2018

Conversation

Projects
None yet
2 participants
@cdupuis
Copy link
Contributor

cdupuis commented Dec 6, 2018

This allows to modify the creation of GraphClient instances through a factory.

The default implementation ApolloGraphClientFactory allows to plug in strategies to modify the internally used fetch instance.

@ddgenome
Copy link
Member

ddgenome left a comment

Looks good. Just some code duplication that should probably be centralized with some TypeDoc on what exactly it is doing since it is a little confusing that commands and events each get their workspace ID from a different place, although both places use "team" rather than workspace.

workspaceId = event.team.id;
} else if (isEventIncoming(event)) {
workspaceId = event.extensions.team_id;
}

This comment has been minimized.

@ddgenome

ddgenome Dec 6, 2018

Member

This logic appears twice in this PR. Should it be centralized and explained?

This comment has been minimized.

@ddgenome

ddgenome Dec 6, 2018

Member

MIssed one.

workspaceId = event.team.id;
} else if (isEventIncoming(event)) {
workspaceId = event.extensions.team_id;
}

This comment has been minimized.

@ddgenome

ddgenome Dec 6, 2018

Member

Three times.

return this.graphClients.create(
workspaceId,
this.configuration,
() => `Bearer ${this.registration.jwt}`);

This comment has been minimized.

@ddgenome

ddgenome Dec 6, 2018

Member

And this twice.

@cdupuis

This comment has been minimized.

Copy link
Contributor Author

cdupuis commented Dec 6, 2018

Good points. Thanks. Fixes coming.

@cdupuis cdupuis force-pushed the graph-client-factory branch from a1d6490 to 60516a2 Dec 6, 2018

@ddgenome
Copy link
Member

ddgenome left a comment

Minor

workspaceId = event.team.id;
} else if (isEventIncoming(event)) {
workspaceId = event.extensions.team_id;
}

This comment has been minimized.

@ddgenome

ddgenome Dec 6, 2018

Member

MIssed one.

Show resolved Hide resolved lib/internal/transport/RequestProcessor.ts

@cdupuis cdupuis force-pushed the graph-client-factory branch from 60516a2 to 9e8d005 Dec 7, 2018

@ddgenome
Copy link
Member

ddgenome left a comment

It would be good to deal with an undefined workspaceId downstream, but perhaps that never really happens.

@cdupuis cdupuis merged commit 94ad0bf into master Dec 7, 2018

2 checks passed

license/cla Contributor License Agreement is signed.
Details
sdm/atomist/atomist-sdm Atomist Software Delivery Machine goals: all succeeded
Details

@cdupuis cdupuis deleted the graph-client-factory branch Dec 7, 2018

atomist-bot added a commit that referenced this pull request Dec 7, 2018

Changelog: #410 to added
[atomist:generated]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment