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: Allow async endpoint options #139

Merged
merged 3 commits into from
Jun 26, 2019
Merged

feat: Allow async endpoint options #139

merged 3 commits into from
Jun 26, 2019

Conversation

amccloud
Copy link
Contributor

@amccloud amccloud commented Jun 20, 2019

To support desktop's async config https://github.com/goabstract/ui/pull/1215 we need to allow for previewsUrl to be async

I went ahead and updated previewsUrl and the remaining configs that may be used asynchronously one day. Was straight forward because everywhere these options are used were already async 👍

  • Change previewsUrl, apiUrl, 'webUrl', and cliPath to string | Promise<string>
  • Upgrade flow to asssit with suppressing spread bug with unions and to match the ui version

@amccloud amccloud added the enhancement New feature or request label Jun 20, 2019
@amccloud amccloud self-assigned this Jun 20, 2019
@@ -54,6 +54,7 @@ export default class Client {
previewsUrl: "https://previews.goabstract.com",
transportMode: "api",
webUrl: "https://app.goabstract.com",
// $FlowFixMe https://github.com/facebook/flow/pull/7298
...options
};
Copy link
Contributor Author

@amccloud amccloud Jun 21, 2019

Choose a reason for hiding this comment

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

re: facebook/flow#7298 (comment)

From Jordan Brown @ Facebook

That's a separate issue, your spread issue will be solved by my value spreads work. I'm hoping that the type spread portion will land soon and then I'll be full steam ahead on value spreads. It's definitely super annoying. If you have suppression comments configured, this is the perfect time to use one!

I upgraded flow so that I could easily $FlowFixMe this. Prior to 0.101 the error showed up 4 times, requiring suppression comments on [1][3][5] and [7]

Cannot spread options because:
 • Promise [1] is incompatible with string [2].
 • Promise [3] is incompatible with string [4].
 • Promise [5] is incompatible with string [6].
 • Promise [7] is incompatible with string [8].

     src/Client.js
 [2]   50│       apiUrl: "https://api.goabstract.com",
       51│       cliPath:
 [4]   52│         "/Applications/Abstract.app/Contents/Resources/app.asar.unpacked/node_modules/@elasticprojects/abstract-cli/bin/abstract-cli",
       53│       maxCacheSize: 0,
 [6]   54│       previewsUrl: "https://previews.goabstract.com",
       55│       transportMode: "api",
 [8]   56│       webUrl: "https://app.goabstract.com",
       57│       // $FlofwFixMe https://github.com/facebook/flow/pull/7298
       58│       ...options
       59│     };
       60│
       61│     this.activities = new Activities(this, options);

     src/types.js
 [1] 1467│   apiUrl: string | Promise<string>,
 [3] 1468│   cliPath: string | Promise<string>,
     1469│   maxCacheSize: number,
 [5] 1470│   previewsUrl: string | Promise<string>,
     1471│   transportMode: "auto" | "api" | "cli",
 [7] 1472│   webUrl: string | Promise<string>

const callArgs = (child_process.spawn: any).mock.calls[0][1].slice(
-args.length
);

Copy link
Contributor Author

@amccloud amccloud Jun 21, 2019

Choose a reason for hiding this comment

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

child_process is now covered and spawn.mock does not exist

@@ -205,7 +206,7 @@ export default class Endpoint {
return response;
}

async _getAPIHeaders(headers?: Object) {
async _getAPIHeaders(headers?: {}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Object === any. Switching to {} where possible is a quick way to expand coverage

Copy link
Contributor

Choose a reason for hiding this comment

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

Object === any – wait, what? 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

) {
const hostname =
overrideHostname !== undefined ? overrideHostname : await this.apiUrl;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🍖

) {
const hostname =
overrideHostname !== undefined ? overrideHostname : await this.apiUrl;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🍖

this.cliPath,
[...tokenArgs, "--api-url", this.apiUrl, ...args]
cliPath,
[...tokenArgs, "--api-url", await this.apiUrl, ...args]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🍖

@@ -33,7 +37,7 @@ export default class Previews extends Endpoint {
"Abstract-Api-Version": undefined
}
},
this.previewsUrl
await this.previewsUrl
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🍖

@amccloud amccloud changed the title feat: Async endpoint options feat: Allow async endpoint options Jun 21, 2019
@bitpshr
Copy link
Contributor

bitpshr commented Jun 21, 2019

@amccloud: Let me know when this is out of draft and ready to review - excited to land this.

@amccloud
Copy link
Contributor Author

amccloud commented Jun 21, 2019

@bitpshr thanks! It’s pretty much ready but having some ci woes.

Not sure if it’s because I recently changed node versions but the integrity hashes are off 🤔

Edit: All set. I had to clear my local .yarn-cache. I wonder if that was caused by the node version change

@amccloud amccloud requested review from bitpshr and tommoor June 25, 2019 18:12
@amccloud amccloud marked this pull request as ready for review June 25, 2019 18:14
Copy link
Contributor

@bitpshr bitpshr left a comment

Choose a reason for hiding this comment

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

Everything looks good to me. Clean change. Thanks for handling this!

@amccloud amccloud merged commit 50dc822 into master Jun 26, 2019
@amccloud amccloud deleted the async-friendly branch June 26, 2019 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants