chore: only load keytar during the migration process VSCODE-450

[kmruiz]

Only load keytar during the migration of stored connections to process to the new SecretStorageAPI, maintained by the VSCode team. This is a requirement specified in VSCODE-450. This is related to #546.

Description

Keytar is removed from the activation of the plugin, and only loaded when the migration process starts.

Checklist

• New tests and/or benchmarks are included
• Documentation is changed or added
• I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

Please, see #546 for more information.

• Bugfix
• New feature
• Dependency update
• Misc

Open Questions

Dependents

Types of changes

• Backport Needed
• Patch (non-breaking change which fixes an issue)
• Minor (non-breaking change which adds functionality)
• Major (fix or feature that would cause existing functionality to change)

[himanshusinghs]

Looks good! 🎉
I was thinking, we probably won't need to assign the created keytar module to ext but that would also entail a bunch of refactoring in our tests. Maybe we can leave it as you have it right now, since it all will anyways be removed at some point.

[alenakhineika]

Should we maybe update this model, so it remembers that keytar require failed once, and we don't try to resolve it again for each next connection that goes through the migration? With our webpack configuration, the failed require won't be cached:

vscode/webpack.config.js

Lines 75 to 76 in 711587b

	strictModuleErrorHandling: true,
	strictModuleExceptionHandling: true,

[alenakhineika]

Another way to handle it is to move the createKeytar call to the loadSavedConnections function, where can see if there are connections without secretStorageLocation and then try to require it once for the whole migration attempt.

[kmruiz]

Which of these solutions you think would be more valuable? From what I've understood, keytar is a deprecated feature, so all the code related to it will be removed at some point. IMO, I would just do whatever is cheaper and doesn't impact the user negatively.

[alenakhineika]

I would require it once in loadSavedConnections, but you can decide what works better implementation-wise.

[kmruiz]

Moving it to loadSavedConnections needs some changes in the tests, as some of them fail.

I'll keep the current version and merge.

Thanks a lot!

[alenakhineika]

What will happen when vscode team removes keytar from vscode and there are migrations that require a migration? Are we ok with resolving the keytar module for each connection? Will this make the launch of the extension slower than it could be?

[alenakhineika]

Moving createKeytar to loadSavedConnections should note brake tests. Have you tried something like this?

async loadSavedConnections(): Promise<void> {
    const globalAndWorkspaceConnections = Object.entries({
      ...this._storageController.get(
        StorageVariables.GLOBAL_SAVED_CONNECTIONS,
        StorageLocation.GLOBAL
      ),
      ...this._storageController.get(
        StorageVariables.WORKSPACE_SAVED_CONNECTIONS,
        StorageLocation.WORKSPACE
      ),
    });

    const hasConnectionsThatDidNotMigrateEarlier =
      !!globalAndWorkspaceConnections.some(([, connectionInfo]) => !connectionInfo.storageLocation);

    if (hasConnectionsThatDidNotMigrateEarlier) {
      try {
        ext.keytarModule =
          ext.keytarModule === undefined ? createKeytar() : ext.keytarModule;
      } catch (err) {
        // Couldn't load keytar, proceed without storing & loading connections.
      }
    }

    ...
}