Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix part of #5002 remove GCS_RESOURCE_BUCKET_NAME from GLOBALS #7130
Fix part of #5002 remove GCS_RESOURCE_BUCKET_NAME from GLOBALS #7130
Changes from 5 commits
7e7435b
3c169e3
a9b1b20
5469744
f9d53ea
435c25a
5f95d2e
f4d9ae3
d79e4ba
197c774
5345bdb
3012216
2efc78f
2ca6ff5
c28854c
4d837e1
bcf0bec
f02a37c
5028a22
25470cf
c832832
f82ad3b
f6ee206
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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's a bit icky that you're calling initialize() multiple times in a single file. Initialization of a service should be done at most once on an entire page.
Perhaps by initialize() you mean something else? Why should initialization ever need to happen twice (conceptually)?
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'm not sure why we have to initialize the service every time, although I've initialized it in the begging. Can we move the
GCS_RESOURCE_BUCKET_NAME
check inside thesaveAudio
method?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 agree with @DubeySandeep -- I'm surprised that you need to reinitialize this each time. That's a bit worrying and needs to be explained (or, ideally, removed).
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 think this was a leftover from a previous implementation.
Removed.
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.
Do we need similar changes for the image upload and retrieval?
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.
No need for this.
It has been removed.
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.
Remove
.initilize()
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.
Move this to line 43.
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.
Add a test for this new method/function.
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'm not sure of what to test exactly, since this function is required for other functions to run, and it is made to run before each test so if it fails, other tests will fail.
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 is this commented out?
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 prefix is only used in PROD
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 looked at the file more carefully and I think I understand what the issue is. Now that we are fetching the bucket name asynchronously, you need to wait for that promise to resolve before you can actually do anything with regards to the assets.
I think the current approach is fraught, though. No developer is going to remember to call initialize() explicitly and there aren't really any rules for when it should be called. So this is not really maintainable, and that's why, typically, a service takes care of it's own initialization. The current approach is a bit scattershot and relies on other files telling this service when to initialize.
Instead, this service should still take care of its own initialization, but calls to this service should still always get the correct responses. Can you find a way to do this such that, if a (presumably async) call to serve stuff comes in when the GCS bucket data hasn't come in yet, that call gets served only when that data has come in? I think I saw a recent PR by you or @brianrodri that did something like this, but I can't remember which one it was -- IIRC it involved storing a promise in a local variable in the service and returning that.
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 have refactored it to initialize itself and the other methods returns a promise which is fulfilled after the bucket name has been fetched.
PTAL.
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.
If you are calling initialize() at the outset, wouldn't GCS_RESOURCE_BUCKET_NAME be null?
If you are not calling initialize() only at the outset, why call the method name "initialize"?
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.
initialize is called where the assets backend service is needed.
the check is done for caching purposes. On prod, after sending the request, the response is being cached and a further call to initialize shouldn't do anything again since the service has already been initialized.
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.
No, that is messy. Initialization of a service should be done exactly once per page.
If you're exposing this method publicly for others to call, then you would need to raise an exception if it's called more than once.
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.
Now that I think of it. Remove this
if
altogether.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 don't think this if should be removed since it validated the result gotten from the handler
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.
Do we need to chain the
.then(
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.
No
Removed. Thanks.
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 do you need to check
DEV_MODE
here? Shouldn't this be handled on line 51?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, removed.
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 this change?
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.
Unnecessary. I think it is due to my editor formatting.
Removed.
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.
These should not be here... (and in the next file).