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.

Sign up for GitHub

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

native platform fixes #951

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

native platform fixes #951

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions GooglePlacesAutocomplete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,22 @@ export const GooglePlacesAutocomplete = forwardRef((props, ref) => {
const [listLoaderDisplayed, setListLoaderDisplayed] = useState(false);

const inputRef = useRef();
const [sessionToken, setSessionToken] = useState(uuidv4());

const randomGenerator = useCallback(() => {
const randomByteArray = [];
for (let i = 0; i < 16; i++) {
const rand = Math.floor(Math.random() * 256);
randomByteArray.push(rand);
}
return randomByteArray;
});

const generateSessionToken = useCallback(() => {
return uuidv4({ rng: randomGenerator });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we had an issue concerning usage of crypto.getRandomValues on iOS Native devices, we can use a random generator for session token generation which I think is good enough for the given scenario. This change works well on iOS devices.

Copy link

Choose a reason for hiding this comment

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

I can't think of any reason these need to be cryptographically secure, can you? If not, this seems fine to me 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't think of any reason these need to be cryptographically secure, can you? If not, this seems fine to me

Yeah. I don't see any valid reason either to make it so secure.

}, [randomGenerator]);

const [sessionToken, setSessionToken] = useState(generateSessionToken());

useEffect(() => {
setUrl(getRequestUrl(props.requestUrl));
}, [getRequestUrl, props.requestUrl]);
Expand Down Expand Up @@ -337,7 +352,7 @@ export const GooglePlacesAutocomplete = forwardRef((props, ref) => {
fields: props.fields,
}),
);
setSessionToken(uuidv4());
setSessionToken(generateSessionToken());
} else {
request.open(
'GET',
Expand Down Expand Up @@ -628,6 +643,10 @@ export const GooglePlacesAutocomplete = forwardRef((props, ref) => {
setRequestHeaders(request, getRequestHeaders(props.requestUrl));

if (props.isNewPlacesAPI) {
request.setRequestHeader(
'content-type',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Android native devices, the API request fails with the response message stating that it needs content-type header. Adding the header resolves the issue with Android native devices.

Copy link

Choose a reason for hiding this comment

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

Can you help me understand why specifically this happens on android and if this is the only fix? As you stated in your App PR, adding this header causes CORS errors - I noticed this on dev too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The response (as shared in screenshot below) precisely pointed out this reason and so didn’t dig deeper. But your question on why specifically this happens on Android is interesting. I will take a closer look at this.

Screenshot 2024-10-09 at 1 23 01 PM

Copy link

Choose a reason for hiding this comment

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

Thanks yeah please do, it is very strange. AFAIK that response is not coming from the server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks yeah please do, it is very strange. AFAIK that response is not coming from the server

@rafecolton That's correct. The response comes from android native implementation as referenced in the screenshot below. Since we have stringified the request body of our POST request, android native implementation expects content-type to be sent. The only way to programmatically avoid this response is to avoid JSON.stringify but it does not seem to be a viable option for us.
So, It looks like we have to address the issue in desktop platform along with the content-type and we are discussing this here

Screenshot 2024-10-29 at 6 33 48 AM

'application/json; charset=utf-8',
);
const { key, locationbias, types, ...rest } = props.query;
request.send(
JSON.stringify({
Expand Down