Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

update queries/commands api to accept references instead of string ids (closes #50 closes #49 closes #52) #51

Merged
merged 18 commits into from
Mar 16, 2018

Conversation

giogonzo
Copy link
Member

@giogonzo giogonzo commented Mar 11, 2018

This PR does a few things:

test plan

Didn't test on a project for the moment, see added tests

@nemobot nemobot added the WIP label Mar 11, 2018
@giogonzo giogonzo changed the title update queries/commands api to accept references instead of string ids (closes #50 closes #49) update queries/commands api to accept references instead of string ids (closes #50 closes #49 closes #52) Mar 12, 2018
@giogonzo giogonzo force-pushed the 50-update_queriescommands_api_to branch from abdc649 to 0bf1e98 Compare March 12, 2018 13:49
@giogonzo giogonzo requested a review from veej March 12, 2018 15:03
@nemobot nemobot added in review and removed WIP labels Mar 12, 2018
@giogonzo giogonzo self-assigned this Mar 12, 2018
@giogonzo giogonzo force-pushed the 50-update_queriescommands_api_to branch from 482565e to 5b7436d Compare March 13, 2018 08:48
Copy link
Member

@veej veej left a comment

Choose a reason for hiding this comment

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

Not sure about the types, everything else looks 👍
minor comments inline

@@ -22,18 +28,42 @@
},
"homepage": "https://github.com/buildo/react-avenger#readme",
"devDependencies": {
"@types/react": "^16.0.40",
Copy link
Member

Choose a reason for hiding this comment

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

should we pin it?

Copy link
Member Author

Choose a reason for hiding this comment

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

why?

it('runs a command forwarding all props', async () => {
const run = jest.fn(Promise.resolve.bind(Promise));
const doFoo = Command({
params: { token: t.string },
Copy link
Member

Choose a reason for hiding this comment

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

Should we add foo here?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 done

@giogonzo giogonzo merged commit 44237d5 into master Mar 16, 2018
@nemobot nemobot removed the in review label Mar 16, 2018
@nemobot
Copy link

nemobot commented Mar 16, 2018

@giogonzo giogonzo deleted the 50-update_queriescommands_api_to branch March 16, 2018 15:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants