The fundamental problem is that you're not handling all the async stuff as though it's async. This is a reasonably complicated workflow, with blocking and non-blocking tasks intermixed, which means that we need to apply asynchronous patterns throughout. Let's step through the script one function at a time.
This appears to be the script's entry point:
export const myStartup = (callback) => {
const token = getJwtToken();
callback(token);
}
It won't work, because getJwtToken is async, which means that its value will not be available to callback on the next line.
How do we know that getJwtToken is async? Because getJwtToken invokes getJwtTokenFromApi, which invokes fetch (which the spec tells us is async). Since getJwtToken wraps async behavior, it is itself async.
Since getJwtToken is async, we know that token is not going to be available on the second line when callback needs it. In fact, token will never be available in that scope, because getJwtToken returns a Promise, whose resolution value will only be available inside a .then handler. So, step 1 is to rewrite this function:
export const myStartup = (callback) => {
getJwtToken() // returns a Promise
.then((token) => { // token is available inside Promise's .then
callback(token);
})
}
Now we look inside getJwtToken, bearing in mind that it must return a Promise because of the changes we just made.
export const getJwtToken = () => {
let token = localStorage.getItem('myToken');
if (token == null)
token = getJwtTokenFromApi();
return token;
}
This is an interesting case, because getJwtToken implements branching behavior, one branch of which is synchronous, and the other not. (localStorage.getItem blocks, but getJwtTokenFromApi is async.) The only way to handle cases like this is to make the entire function async: to make sure that it always returns a Promise, even if the data it needs is available from a sync source.
Since localStorage.getItem is synchronous, if we like the value it gives us, we wrap that value in a Promise before returning it. Otherwise, we can just return the Promise returned by getJwtTokenFromApi:
export const getJwtToken = () => {
let token = localStorage.getItem('myToken')
if(token !== null)
return Promise.resolve(token);
return getJwtTokenFromApi();
}
Now, no matter which scenario we find ourselves in, this function will return a Promise that contains a token.
Finally, we get to getJwtTokenFromApi, which does a few things:
- it constructs a
Request
- it executes a request (async)
- if successful, it converts the response to text (async)
- it inspects the text
If all those things work out, it wants to return the text value. But half of those tasks are async, which again means that the entire function must become async. Here's a slimmer version of what you started with:
export const getJwtTokenFromApi = () => {
var request = new Request('/api/token', {});
fetch(request)
.then((response) => {
response.text()
.then((token) => {
if(token.length > 0) {
localStorage.setItem('myToken', token);
return token;
} else {
return null;
}
})
})
}
The biggest problem here is that you're not returning the fetch. This is important, because the other return statements nested inside don't apply to the overall function. This function will not return anything as written, although it will perform an XHR call. So, the first fix is to return fetch.
But just adding that return isn't enough. Why? Because within the .then handler, you want to access the text of the response, but that access is itself async. While you are using a .then to access the value (as token), that value will die silently inside fetch.then unless you also return response.text(). Really, what you need is this:
return fetch(request)
.then((response) => {
return response.text()
.then((text) => {
if(text.length > 0) return text;
else return null
But this code is needlessly verbose, and the way it creeps to the right with deeper and deeper nesting makes for code that is hard to read or re-order. These steps are sequential, and we want them to look that way:
STEP 1
STEP 2
STEP 3
(not)
STEP 1
STEP 2
STEP 3
So, let's try something slimmer:
return fetch(request) // step 1
.then((response) => response.text()) // step 2
.then((text) => text.length > 0 ? text : null) // step 3
This code is flatter and slimmer. It's also easier to re-order the steps or insert new ones. Of course, it doesn't do the important work of storing the token in localStorage, which is why we have the slightly beefier final version:
export const getJwtTokenFromApi = () => {
var request = new Request('/api/token', {
method: 'GET',
mode: 'cors',
credentials: 'include'
});
return fetch(request)
.then((response) => response.text())
.then((token) => {
if(token.length > 0) {
localStorage.setItem('myToken', token);
return token;
}
return null;
})
})
}
We're able to flatten all this code because of the way nested Promises resolve: when one Promise contains another Promise (and another, etc.), the engine will automatically unwrap all of the intermediate promises. As an example, these two snippets produce identical results:
var x = Promise.resolve( Promise.resolve( Promise.resolve ( 10 )))
var y = Promise.resolve( 10 )
Both x and y will act like single, flat Promises that resolve to 10, which means we can put this after either one:
.then((value) => {
// value === 10
})
Here's the final script:
export const getJwtTokenFromApi = () => {
var request = new Request('/api/token', {
method: 'GET',
mode: 'cors',
credentials: 'include'
});
return fetch(request)
.then((response) => response.text())
.then((token) => {
if(token.length > 0) {
localStorage.setItem('myToken', token);
return token;
}
return null;
})
})
}
export const getJwtToken = () => {
let token = localStorage.getItem('myToken')
if(token !== null)
return Promise.resolve(token);
return getJwtTokenFromApi();
}
export const myStartup = (callback) => {
getJwtToken()
.then((token) => {
callback(token);
})
}
One more question: is myStartup async or not?
Using the rule of thumb from above, we'd say that since it wraps async behavior, it is itself async. However, this script mixes async patterns: both Promises & callbacks. I suspect this is because you are more familiar with node-style callbacks one the one hand, but fetch returns a Promise, and during implementation those two approaches kind of "meet in the middle" -- or rather, at the module's API: myStartup. It's an async function, but it doesn't seem comfortable with that fact.
When a caller invokes myStartup, it will return nothing. That much is obvious because there is no return statement. But, by accepting a callback function, you've provided a mechanism to signal callers once all the potentially-async work is complete, which means it can still be used.
Unless it's important to support the node-style callback pattern, I'd recommend taking the final step to make this module thoroughly Promise-based: modify myStartup so that it returns a Promise that resolves with the token. Because of the aforementioned unwrapping behavior, this is an extremely simple change:
export const myStartup = () => {
return getJwtToken();
}
But now it's obvious that myStartup adds nothing to the process, so you might as well remove the wrapper by deleting the function and renaming getJwtToken to myStartup.
