Downgrade keytar dependency for broader compatibility

[colin-grant-work]

What it does

Fixes #9640 by downgradiing keytar to the version currently used by VSCode.

How to test

Follow steps in #9463:

1. Clone, compile and start the sample project: https://github.com/vinokurig/secrets-test.
2. Run Get Password command and see a notification with empty (undefined) password.
3. Run Set Password command and see a notification about password change.
4. Run Get Password command and see a notification with the updated password.
5. Run Delete Password command and see a notification about password change.
6. Run Get Password command and see a notification with empty (undefined) password.

Now do it on RHEL/CentOS7 and see that you don't get any runtime failures.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

Signed-off-by: Colin Grant [email protected]

[vince-fugnitto]

I confirmed that the changes:

• work correctly for both the browser and electron targets
• are aligned with the version present in vscode

I do not have a RHEL/CentOS7 machine to test but I believe you do so I trust it also now fixes issues with older distros.

[paul-marechal]

I'd rather merge: #9707

The reasoning is that the failures you have encountered are to be expected, to some extend.

The solution I propose is to configure your environment to build node-keytar in your target environment.

[colin-grant-work]

@paul-marechal, I'm fairly neutral on whether to merge this or #9707. I see these as the main arguments:

For #9707:

• We get the latest version of dependencies, which is usually good.
• Pinning a dependency to an earlier version mostly defers the question - eventually you'll likely want to upgrade.

For #9694:

• Lower risk of failure in the worst case, less configuration in the best case for affected users (likely not to be a large group)
• keytar doesn't seem to have done much in its last few releases
• Fix/document some of keytar issues #9707 (comment)

The discussion applies to all of our native modules. The latest yarn.lock upgrade broke oniguruma and drivelist in our environment, as well, and we haven't found a setup other resolving them to older versions that gets all three (including keytar) to work at the same time.

[tsmaeder]

@colin-grant-work @paul-marechal binding the code to old dependency versions seems like a bad practice unless we have a policy on supported versions. Can we structure the code in a way that adopters that need to run on older distros can easily override the version of keytar used (i.e. put the native lib dependency in it's own extension module)?

[colin-grant-work]

@tsmaeder, it's very easy for adopters to just add resolutions to their package.json to override whatever is specified elsewhere - that's what we've done internally. I'm fine with that solution in general, as well, as long as we document what workarounds might be necessary somewhere. @paul-marechal has been working on a PR to document ways to actually make keytar play nice (#9707), but has run into some bothersome issues there. If we want to be less ambitious and just tell people how they may need to pin their versions for particular OS's, that would make #9707 simple. It's important to be aware of these issues for the blueprint as well, (@JonasHelming) because if you build a package with the newer versions of native dependencies and someone tries to run it on an OS (version) that doesn't support those versions, they will have a runtime failure.

[tsmaeder]

I would be interested to know why we are using this new version of keytar. Since the downgrade works, I guess we're not using any features from the new version. Also: shouldn't keytar handle the problem of running on older versions of libsecret?

[colin-grant-work]

I would be interested to know why we are using this new version of keytar. Since the downgrade works, I guess we're not using any features from the new version. Also: shouldn't keytar handle the problem of running on older versions of libsecret?

I think the 'why' is just that it's the latest version, so when we added it to our dependencies, that's what we went with. There's been almost no change in actual keytar code base in a while, so downgrading doesn't really have us missing out on any features.

As to why keytar doesn't handle different environments well, @paul-marechal can likely speak more definitively, but my impression is that there are a couple of ways native modules get built: they can build themselves locally, or they can have a set of binaries associated with different versions for users to download. By default, keytar follows the second approach, and its latest binaries assume particular versions of the standard library. Paul's had some luck getting it to build locally, in which case it's happy with whatever it finds. But that approach seems to run into problems, and we've had trouble getting all of the native dependencies to work together after the latest changes to our yarn.lock, so we're sticking with resolutions.

[paul-marechal]

Fine with downgrading, no strong opinion anymore. Had enough pain with building the latest version of keytar on my end.

[vince-fugnitto]

The changes look good to me and solve issues that downstream apps may experience 👍
I think we should aim to include it for tomorrow's 1.16.0 release.

The CI failures should be resolved when rebased.

[colin-grant-work]

I think we should aim to include it for tomorrow's 1.16.0 release.

Tomorrow's release includes other dependency changes that could cause problems, including version bumps to drivelist and oniguruma. Should we bump those back down to the versions at the last release? This goes to the question that @tsmaeder posed in the dev meeting about what we plan to support - and how we plan to support it.

[paul-marechal]

including version bumps to drivelist and oniguruma

Which PR(s) bumped those?

[colin-grant-work]

Which PR(s) bumped those?

Looks like I'm wrong about drivelist - not sure how it got on my list - but oniguruma was bumped by @vince-fugnitto 's recent yarn.lock upgrade:

In our RHEL7 environment, we've pinned it to the previous version, 7.2.1. I haven't looked as closely at the cause of the failure as I have for keytar. It fails during the build with a complaint from node-gyp, but more than that I can't say for sure at the moment.

[vince-fugnitto]

Looks like I'm wrong about drivelist - not sure how it got on my list - but oniguruma was bumped by @vince-fugnitto 's recent yarn.lock upgrade:

@colin-grant-work the yarn.lock is updated to better reflect what downstream apps may use given our dependency version ranges. If our version range is too broad where we have incompatibilities we should think of possibly narrowing it down. Unfortunate that a patch version would cause issues.