Fix/asl visualizer no internet #3098
Conversation
This fix allows for a graceful fallback when internet connectivity isn't available to a c9 or vscode host but where the required files are available in the local filesystem cache (either by previous automatic caching, or by manual user intervention). Implementation avoids confusing the return type of 'updateCache' or 'updateCachedFile' and instead only checks for local files after a updateCache call fails.
…sualization Issue: aws#2922. Step Function visualization fails even if graph.js & graph.css are placed in local directory for use.
| } catch (err) { | ||
| // So we can't update the cache, but can we use an existing on disk version. | ||
| try { | ||
| logger.warn( | ||
| 'Updating State Machine Graph Visualisation assets failed, checking for fallback local cache.' | ||
| ) | ||
| await this.cache.confirmCacheExists() | ||
| } catch (err) { | ||
| logger.error('No local cached State Machine Graph Visualization assets found.') | ||
| throw err | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we move this logic to StateMachineGraphCache.updateCache and tweak the log messages a bit? That way AslVisualizationCDKManager would benefit from this fix too. Or we could add a new method to AbstractAslVisualizationManager and call that instead of this.cache.updateCache
There was a problem hiding this comment.
Hi @JadenSimon , thanks for the review.
I'm very happy to move the logic, my concern was messing up the return meaning of updateCache i.e. it would return positive/no errors even though the cache wasn't updated.
I'll have a look at AbstractAslVisualizationManager as hopefully a better pathway.
There was a problem hiding this comment.
Makes sense to me. AbstractAslVisualizationManager is a better place if we don't want to change semantics of existing code.
| const err = new Error('Failed to located cached State Machine Graph assets') | ||
| throw err |
There was a problem hiding this comment.
nit:
| const err = new Error('Failed to located cached State Machine Graph assets') | |
| throw err | |
| throw new Error('Failed to located cached State Machine Graph assets') |
| }) | ||
| if (cssExists && jsExists) { | ||
| return true | ||
| } else { |
There was a problem hiding this comment.
nit: no need to use else if the other branch returns
| const cssExists = await this.confirmCachedFileExists({ | ||
| filePath: this.cssFilePath, | ||
| }) | ||
| const jsExists = await this.confirmCachedFileExists({ | ||
| filePath: this.jsFilePath, | ||
| }) |
There was a problem hiding this comment.
confirmCachedFileExists seems unnecessary
| const cssExists = await this.confirmCachedFileExists({ | |
| filePath: this.cssFilePath, | |
| }) | |
| const jsExists = await this.confirmCachedFileExists({ | |
| filePath: this.jsFilePath, | |
| }) | |
| const cssExists = await this.fileExists(this.cssFilePath) | |
| const jsExists = await this.fileExists(this.jsFilePath) |
| this.logger.error('Failed to locate cached State Machine Graph css assets') | ||
| // This log message is intended to help users setup on disconnected C9/VSCode instances. | ||
| this.logger.error(`Expected to find: "${VISUALIZATION_CSS_URL}" at "${this.cssFilePath}"`) |
There was a problem hiding this comment.
log messages should try to stay "atomic" if possible
| this.logger.error('Failed to locate cached State Machine Graph css assets') | |
| // This log message is intended to help users setup on disconnected C9/VSCode instances. | |
| this.logger.error(`Expected to find: "${VISUALIZATION_CSS_URL}" at "${this.cssFilePath}"`) | |
| // This log message is intended to help users setup on disconnected C9/VSCode instances. | |
| this.logger.error(`Failed to locate cached State Machine Graph css assets. Expected to find: "${VISUALIZATION_CSS_URL}" at "${this.cssFilePath}"`) |
|
Thank you @simis2626 for your great work here! I've picked up your original code, incorporated the excellent review comments & suggestions from @justinmk3 and @JadenSimon and opened a new PR #3485 containing all the changes. I squashed all these commits for a clean commit log, but you're still credited as the author - great first contribution, thank you so much! Once that PR is merged, the maintainers can close this PR. |
PR for #2922 - sfn render fails if host has no internet connectivity, even with cached files present locally.
Problem
Step Function Visualizer fails to render graph due to trying to source the graph.js and graph.css files from the internet on an disconnected c9/code instance - even when those files are available locally.
Solution
On failure of network connectivity, falls back to local files where they both exist.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.