-
Notifications
You must be signed in to change notification settings - Fork 2
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 proxy code #1
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general recommendation here: please try to refactor the code to isolate different cases and comments about each case. Currently, it is really hard to understand what is parsed, when and how.
- run: npm run format | ||
- run: git status | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: fix trailing space and keep only one empty line
.gitignore
Outdated
@@ -0,0 +1,2 @@ | |||
node_modules/ | |||
node_modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node_modules/ should be enough
- add empty line
decodedURI = decodeURIComponentTillSame(decodedURI); | ||
return decodedURI; | ||
} | ||
function decodeURIComponentTillSame(uri) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the idea behind running decodeURI() in a loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes JavaScript decode utility doesn't decode the url in one go. For some URIs, the request throws highly encoded URIs which have to be decoded further to make a further request or extract information from them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please provide a link to documentation that describes this limitation of decodeURI()? Yes, I understand that a URL can be encoded twice or even thrice, but is it a case?
src/coordinates.ts
Outdated
DNT: '1', | ||
}, | ||
}) | ||
.then((value1) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this block do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It resolves the async promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A silly question: why do we have both ".then((value1) => {" and " .then(async (value2) => {"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I am not following why both these then
blocks are needed. Can we say const resp = await fetch(request.query.url, {
and then use resp.url
?
src/coordinates.ts
Outdated
lati = final.plus_code.geometry.location.lat; | ||
lng = final.plus_code.geometry.location.lng; | ||
} catch { | ||
if (host === 'www.google.com' || host === 'maps.google.com') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what case is handled here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in the code below this also the case handled here is when the coordinates are stored in a link which contains ";markers" in it which are located in the backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised that we check the host only here. Shouldn't it be checked at the beginning of the function? This link extended, maybe extended for other providers in the future.
src/coordinates.ts
Outdated
console.log(global_code); | ||
var global_code = result.plus_code.global_code; | ||
console.log(global_code); | ||
var pc_final = global_code.substring(0, 4) + plusCode.substring(0, plusCode.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic offsets. Please provide an example of strings parsed in a comment.
src/coordinates.ts
Outdated
lati = lati.toString() | ||
lng = lng.toString() | ||
} catch { | ||
console.log('third') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many nested levels... could you please describe possible cases here in bare English to understand how to streamline this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can create different functions and in place of try catch we can call these functions?
src/coordinates.ts
Outdated
console.log('fifth') | ||
let searchString = 'https://www.google.com/maps/search/' | ||
position = searchString.length + results.indexOf('https://www.google.com/maps/search/') | ||
link = results.substring(position,position+250) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, magic offsets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add more comments for better readability and understanding
src/coordinates.ts
Outdated
try { | ||
console.log('fourth') | ||
position =results.indexOf('https://www.google.com/maps/place/') | ||
link = results.substring(position - 1, position + 250); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
magic offsets
Signed-off-by: Kartikay <[email protected]>
Signed-off-by: Kartikay <[email protected]>
src/coordinates.ts
Outdated
var lng = null; | ||
var urllen = request.query.url.length; | ||
var string = ''; | ||
for (const key in request.query) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain the logic behing this code?
src/coordinates.ts
Outdated
DNT: '1', | ||
}, | ||
}) | ||
.then((value1) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A silly question: why do we have both ".then((value1) => {" and " .then(async (value2) => {"?
src/coordinates.ts
Outdated
const array = url.split('/'); // Stores an array which contains parts of the url which are separated if they have an "/" between them. Example:- ""https://maps.google.com?q=VGR4+5MC+Falafel+M.+Sahyoun,+Beirut,+Lebanon&ftid=0x151f16e2123697fd:0x8e6626b678863990&hl=en-US&gl=tr&entry=gps&lucs=47067413&g_ep=CAISBjYuNjQuMxgAINeCAyoINDcwNjc0MTNCAlJV" is split into array whose array[arrLength-1] will be "maps.google.com?q=VGR4+5MC+Falafel+M.+Sahyoun,+Beirut,+Lebanon&ftid=0x151f16e2123697fd:0x8e6626b678863990&hl=en-US&gl=tr&entry=gps&lucs=47067413&g_ep=CAISBjYuNjQuMxgAINeCAyoINDcwNjc0MTNCAlJV" (in some URIs this part is even smaller) as you can see this part contains plus code and address in this specific use case which will be utilised further. | ||
const arrLength = array.length; | ||
try { | ||
// PLUS CODE MECHANISM UTILISED:- In the following lines of code firstly the address is separated from plus code. The plus code we get is a shortened plus code so it won't be of use untill we expand it. For this we need to understand plus code making mechanism in itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How URLs with pluscodes are detected? The code above assumes that q=
parameters is a plus code. Wasn't it better to check that q=
is a plus code before starting to parse it?
src/coordinates.ts
Outdated
return response.json(); | ||
} | ||
const response = await fetch(`https://geocode.maps.co/search?q=${cityandState}`); // This send an API request to retreive the center of the city. | ||
const results = await gatherResponse(response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why await response.json()
is not enough?
src/coordinates.ts
Outdated
return response.text(); // Returns the HTML code. | ||
} | ||
var response = await fetch(url); | ||
var results = await gatherResponse(response); // results contains the HTML code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto: await response.text().
src/coordinates.ts
Outdated
// The logic of the proxy is that in the back HTML code of the site, there may lie some different URIs which contain the coordinates, although it is hard to predict which one will be there so we check one by one by using try catch blocks. Also extracting coordinates from plus codes is also utilised which may not require the back HTML code. | ||
const cheerio = require('cheerio'); | ||
|
||
export async function getCoordinates(request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about splitting this function to separate routines for each use-case? The current version is too big and hard to read.
src/coordinates.ts
Outdated
lati = final.plus_code.geometry.location.lat; | ||
lng = final.plus_code.geometry.location.lng; | ||
} catch { | ||
if (host === 'www.google.com' || host === 'maps.google.com') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised that we check the host only here. Shouldn't it be checked at the beginning of the function? This link extended, maybe extended for other providers in the future.
src/coordinates.ts
Outdated
lati = decodeURIComponent(decodeURIComponent(lati.toString())).trim(); | ||
lng = decodeURIComponent(decodeURIComponent(lng.toString())).trim(); | ||
console.log(lati, lng); | ||
if (lati.charAt(0) === '+') { // Sometimes coordinates are extracted as +24.678,89.909 or +23.546,-12.845. And in these type of coordinates a positive or negative sign accompanies them, while the negative sign seems to fit with the geo URI scheme (because of negative coordinates), the positive sign isn't so we have to remove the positive sign. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
normalization of coordinates can be extracted into a separate function
Signed-off-by: Kartikay <[email protected]>
src/coordinates.ts
Outdated
reqBody = null; | ||
lati = null; | ||
lng = null; | ||
urllen = request.query.url.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line fails if no url=
passed. You can return 400 Bad Request here for this case.
http://127.0.0.1:8787/coordinates
src/coordinates.ts
Outdated
return decodedURI; | ||
} | ||
|
||
async function gatherResponse(response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is defined twice.
@@ -0,0 +1,24 @@ | |||
const axios = require('axios'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
axios
is not in "devDependencies"
in package.json
src/coordinates.ts
Outdated
let position; | ||
let results; | ||
|
||
async function gatherResponse(response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wrapping function is not really useful. Please just replace async gatherResponse(response)
with async response.json()
and the same for gatherResponseText()
.
src/coordinates.ts
Outdated
@@ -0,0 +1,371 @@ | |||
const cheerio = require('cheerio'); | |||
|
|||
let urlObj; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using global variables is considered a bad practice, in any programming language. Please define variables where they are needed (by using let
). Use function arguments to pass all values that are needed for the work to do its job. Avoid global state.
console.log(arrLength); | ||
const qParam = urlObj.searchParams.get('q'); // This gets the qParam from the above mentioned part of URL and then splits them apart into plus code and address | ||
if (qParam) { | ||
console.log(array[arrLength - 1].split('=')[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any examples please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
link = decodeURITillSame(address); | ||
console.log('here2'); | ||
var cityandState = link.split(',')[link.split(',').length - 2] + link.split(',')[link.split(',').length - 1]; // Now this part stores the city and state/province of which the address is of. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, please provide any example of a string that is being parsed here.
console.log(response); | ||
var lat = results[0].lat; | ||
var lon = results[0].lon; | ||
const res = await fetch(`https://plus.codes/api?address=${lat},${lon}&[email protected]`); // Utilises the latitude and longitude to retreive the plus codes of the center of the city. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is email required? Can we remove it? If not, please create a global const.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's given in the documentation. I would create a global constant.
src/coordinates.ts
Outdated
var lat = results[0].lat; | ||
var lon = results[0].lon; | ||
const res = await fetch(`https://plus.codes/api?address=${lat},${lon}&[email protected]`); // Utilises the latitude and longitude to retreive the plus codes of the center of the city. | ||
const result = await gatherResponse(res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here and in all other places:
const result = await gatherResponse(res); | |
const result = await res.json(); |
const res = await fetch(`https://plus.codes/api?address=${lat},${lon}&[email protected]`); // Utilises the latitude and longitude to retreive the plus codes of the center of the city. | ||
const result = await gatherResponse(res); | ||
var global_code = result.plus_code.global_code; | ||
var pc = link.split('+')[0] + '%2B' + link.split('+')[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is going on here? Please elaborate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally the plus codes retreived from the URIs are short codes, example JJXX+HR8, Seattle contrary to the long plus codes example 7MQ6GVR2+99. These contain a part of code and the address nearby. The JJXX+HR8 denotes the accuracy of the coordinates upto last decimal in address namely Seatltle. So I use Geocoding APIs to retrieve the centre of the address provided along with the plus code, which is Seattle over here and change the last part of code to prepare the plus code of our location.
For example "Q5RX+VX6, New Colony, Ayodhya, Uttar Pradesh 224123" is the plus code returned by the URI, Now I utilise the address via Geocoding API to retrieve the centre plus code of "New Colony, Ayodhya, Uttar Pradesh 224123" which returns "7MR4Q6V3+QV". Now I use "7MR4Q6V3+QV" and "Q5RX+VX6" (which was earlier received ) to prepare the plus code which is accurate which is "7MR4Q5RX+VX6".
Now this step is a little bit lengthy and can be substituted via Google API but for unshortening short plus codes it has a paid API. I also looked up npm packages but didn't find one that suits our requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding that var pc = link.split('+')[0] + '%2B' + link.split('+')[1];
basically takes 7MR4Q6V3+QV
and replaces it with 7MR4Q6V3%2BQV
(an url encoded version).
Signed-off-by: Kartikay <[email protected]>
Signed-off-by: Kartikay <[email protected]>
No description provided.