Fix or workaround logical and cosmetic bugs with S3 impl #1134
Conversation
- Add locking for (initial call to) GetChildren and (all calls to) LoadMoreChildren Both of these append to the internal node cache Any combination of these calls can possibly be in progress concurrently The easiest (and probably most correct) behavior is to queue up calls behind a single write lock - Ignore attempts to load more children when one is already in progress for the node This can happen if the user double clicks the load more node The intent was most likely to load a single page Rather than queueing up 2 pages, it's better to drop the subsequent load command invocations - Remove leading period from file extension filter Per electron doc, the extension filter should not contain the leading period Without this, Windows was shown to duplicate the file extension - Tweak order of file extension filters Was shown on mac that the first item will be the one selected by default - Use hardcoded SVGs when the user has Seti file icon theme enabled Indentation of child nodes gets messed up when the parent node doesn't have an icon microsoft/vscode#85654 This is the case for the very common Seti file icon theme Switching to SVG works around the issue for Seti Other themes may also not have folder icons however there is no way to detect this currently and the thought is that this is quite uncommon The pros of using the native icon theme outweigh the cons here - Clear the AWS SDK S3 bucket region cache before invocations S3 maintains an internal cache of region to bucket name as an optimization however this cache does not contain the partition Bucket names are not unique across partitions This can cause the wrong region to be selected when the user switches between partitions that contain buckets with the same name - Workaround logging errors when Error.name is a number e.g. AWS SDK for JS can sometimes set the error name to the error code number (like 404) Node.inspect (used by the logger) has a bug where it expects the name to be a string and will actually throw an Error otherwise VSCode's ES5 type definitions and lodash also make this expectation The spec seems to make no mention of the string requirement Additionally, Node fixed the Error in v12.14.1 nodejs/node#30572 However, VSCode uses an older version of Node without the fix
| @@ -116,3 +116,8 @@ export function getChannelLogger(channel: vscode.OutputChannel): ChannelLogger { | |||
| }), | |||
| }) | |||
| } | |||
|
|
|||
| export function isFileIconThemeSeti(): boolean { | |||
| const iconTheme = vscode.workspace.getConfiguration('workbench').get('iconTheme') | |||
There was a problem hiding this comment.
Would it make sense to try/catch wrap this to future proof users in case a future VS Code version is incompatible with this check?
There was a problem hiding this comment.
Double checked the behavior of vscode.workspace.getConfiguration. It will always return a configuration object even if the configuration does not exist. It acts like an empty configuration. So looks like we're always safe to chain these kinds of lookups.
There was a problem hiding this comment.
That's the implementation today, but it could change at a later time. Leaving it to your discretion though.
There was a problem hiding this comment.
Yeah, based on their doc, I would consider that a breaking change.
@return The full configuration or a subset.
And makes no mention of errors thrown.
Even if it weren't as clear in the doc, changing the behavior of any public API should be considered a breaking change. Users can and will code against the observed behavior at the time of implementation.
This reminds me of an example when we had a service that said its API could return any value in the given enum. But in practice it only ever returned X or Y. We made a behavior change where it started to return Z. At least 1 client was unable to handle Z and it turned into a much larger change and rollout process than we anticipated.
Moral was basically that you really need to advertise and prepare clients for behavior changes, even if the behavior is within the specification (or the behavior was previously undefined). Even if you explicitly state that something is possible, if it doesn't occur in practice and you don't explicitly state how to interpret the outcome, clients will not (and can't reasonably be expected to) code for that specific scenario.
- Add locking for (initial call to) GetChildren and (all calls to) LoadMoreChildren Both of these append to the internal node cache Any combination of these calls can possibly be in progress concurrently The easiest (and probably most correct) behavior is to queue up calls behind a single write lock - Ignore attempts to load more children when one is already in progress for the node This can happen if the user double clicks the load more node The intent was most likely to load a single page Rather than queueing up 2 pages, it's better to drop the subsequent load command invocations - Remove leading period from file extension filter Per electron doc, the extension filter should not contain the leading period Without this, Windows was shown to duplicate the file extension - Tweak order of file extension filters Was shown on mac that the first item will be the one selected by default - Use hardcoded SVGs when the user has Seti file icon theme enabled Indentation of child nodes gets messed up when the parent node doesn't have an icon microsoft/vscode#85654 This is the case for the very common Seti file icon theme Switching to SVG works around the issue for Seti Other themes may also not have folder icons however there is no way to detect this currently and the thought is that this is quite uncommon The pros of using the native icon theme outweigh the cons here - Clear the AWS SDK S3 bucket region cache before invocations S3 maintains an internal cache of region to bucket name as an optimization however this cache does not contain the partition Bucket names are not unique across partitions This can cause the wrong region to be selected when the user switches between partitions that contain buckets with the same name - Workaround logging errors when Error.name is a number e.g. AWS SDK for JS can sometimes set the error name to the error code number (like 404) Node.inspect (used by the logger) has a bug where it expects the name to be a string and will actually throw an Error otherwise VSCode's ES5 type definitions and lodash also make this expectation The spec seems to make no mention of the string requirement Additionally, Node fixed the Error in v12.14.1 nodejs/node#30572 However, VSCode uses an older version of Node without the fix
fixed in microsoft/vscode#85657 (not released yet) |
* Use codeWhisperer as a proxy client for WeaverBird * Remove leftovers from direct lambda invocation * Update sessionState tests to reflect cwspr proxy apis * Use new CWSPR APIs * Fix createUploadUrl request payload and isolate logic for fetching resultArchive * Add requestId to the error logs in proxyClient * Fix sessionState tests --------- Co-authored-by: Joao Salles <jvsalles@amazon.de>
Description
Add locking for (initial call to) GetChildren and (all calls to) LoadMoreChildren
Both of these append to the internal node cache
Any combination of these calls can possibly be in progress concurrently
The easiest (and probably most correct) behavior is to queue up calls behind a single write lock
Ignore attempts to load more children when one is already in progress for the node
This can happen if the user double clicks the load more node
The intent was most likely to load a single page
Rather than queueing up 2 pages, it's better to drop the subsequent load command invocations
Remove leading period from file extension filter
Per electron doc, the extension filter should not contain the leading period
Without this, Windows was shown to duplicate the file extension
Tweak order of file extension filters
Was shown on mac that the first item will be the one selected by default
Use hardcoded SVGs when the user has Seti file icon theme enabled
Indentation of child nodes gets messed up when the parent node doesn't have an icon
microsoft/vscode#85654
This is the case for the very common Seti file icon theme
Switching to SVG works around the issue for Seti
Other themes may also not have folder icons however there is no way
to detect this currently and the thought is that this is quite uncommon
The pros of using the native icon theme outweigh the cons here
Clear the AWS SDK S3 bucket region cache before invocations
S3 maintains an internal cache of region to bucket name as an optimization
however this cache does not contain the partition
Bucket names are not unique across partitions
This can cause the wrong region to be selected when the
user switches between partitions that contain buckets with the same name
Workaround logging errors when Error.name is a number
e.g. AWS SDK for JS can sometimes set the error name to the error code number (like 404)
Node.inspect (used by the logger) has a bug where it expects the name to be a string
and will actually throw an Error otherwise
VSCode's ES5 type definitions and lodash also make this expectation
The spec seems to make no mention of the string requirement
Additionally, Node fixed the Error in v12.14.1
nodejs/node#30572
However, VSCode uses an older version of Node without the fix
Motivation and Context
Related Issue(s)
Testing
Screenshots (if appropriate)
Checklist
npm run newChangeLicense
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.