Open Icantjuddle opened 1 year ago
Thanks for reporting this. I tried this out on a GHE instance with ${hostname}/api
for these fetches you pointed out and I ended up getting a CORS error, while api.${hostname}
worked fine (/api/v3
also didn't work for me). So I guess this somehow depends on your particular GHE instance.
Not sure what we should do here if one format works for some instances but not for others and vice-versa.
cc @bolinfest in case you have ideas
@Icantjuddle out of curiosity, have you tried building ReviewStack locally and making the necessary change to see if it works with your GHE instance?
@evangrayk I'll try to repro. I'm surprised ${hostname}/api
would yield a CORS error since we are using https://${hostname}/api/graphql
for GraphQL (as opposed to REST) requests, which seems to work fine? To clarify, instead of:
const url = `https://api.${this.hostname}/repos/${encodeURIComponent(
you tried:
const url = `https://${this.hostname}/api/v3/repos/${encodeURIComponent(
@bolinfest
out of curiosity, have you tried building ReviewStack locally
Yes I have been, was hoping to validate this more fully before filing the issue; however, unfortunately I'm not very familiar with the javascript ecosystem and have been struggling to build it correctly with changes. Here is what I'm doing, and what I've tried, maybe you can see what I'm doing wrong.
addons/reviewstack
yarn codegen
(not sure this is needed)addons/reviewstack.dev
to yarn build
build
folder with twisted
with a self-signed cert (I'm presuming I need to serve over TLS so CORS is happy, since our GHE is only served over TLS).The problem I've noticed is that the requests are still going to the api.${hostname}
even after I've changed those strings (verified in the browser console). So I think somehow the build in addons/reviewstack.dev
is not using the changes I've made to addons/reivewstack
. I noticed in addons/reviewstack.dev/package.json
that it specifies a version of reviewstack
so presumably it is using that over the one in the repo. I googled around and found that yarn link
should be able to help here so I did the following with no result:
yarn link
in addons/reviewstack
yarn link reviewstack
in addons/reviewstack.dev
addons/reviewstack.dev
and tried the above process again.This however did not help and it was still using the old request url: api.${hostname}
.
If you know a way I can get the build picked up correctly please LMK and I would be happy to test.
I was able to verify my suggestion (not the code change) in a different way though. I installed a browser extension requestly and configured it to rewrite URLS matching api.${hostname}
to ${hostname}/api/v3/
and the CORS errors go away and I see the main body of the PR view rendered. However, I still do not see my file diff from the PR rendered in that view, but I'm presuming that is a separate issue.
@bolinfest any thoughts on ^?
@bolinfest just checking in, any thoughts?
@bolinfest @evangrayk Just checking in again, would be happy to put in a PR if you can help me with the local build/test process per the msg above.
Main github.com has its rest apis at
api.github.com
whereas GHE has them atexample.com/api/v3
. This fact is used in a few places to correctly deduce the format but not everywhere. Github has a note about which URL to use here:Correct: https://github.com/facebook/sapling/blob/2f15e589a5b2073546b223656c920fa837fe0bb6/addons/reviewstack/src/github/gitHubCredentials.ts#L253-L273
Not working: https://github.com/facebook/sapling/blob/2f15e589a5b2073546b223656c920fa837fe0bb6/addons/reviewstack/src/github/GraphQLGitHubClient.ts#L173
https://github.com/facebook/sapling/blob/2f15e589a5b2073546b223656c920fa837fe0bb6/addons/reviewstack/src/github/GraphQLGitHubClient.ts#L220