search

kblincoe / VisualGit_SE701_2019_1

1 stars 0 forks source link

Fixes #269 - Improving sign in/out #302

Closed Z-Qi closed 5 years ago

Z-Qi commented 5 years ago

Related Issue/Keyword:

Closes #269

Description:

Now that we have auto sign-in, the load credentials button has become redundant. To reflect this change, as discussed in #269, credentials are also cleared on signing out.

Testing:

Steps for manual testing:

  1. On signing out, load credentials button should no longer appear
  2. After signing out, restarting the application will not automatically sign you in

Checklist:

joel-clarke commented 5 years ago

Just tested this Windows 10 and received the same error. Logged in just like normal, then attempted to sign out and the error popped up. Like @lukethompsxn after dismissing the error, it worked fine.

image

Z-Qi commented 5 years ago

Have tried multiple different approaches, but have yet been unable to find a nice solution to this problem. With help from @AbhinavBehal, we think the issue comes from the generated credentials.js file not having the function as it gets built/rebuilt after index.js. This causes the initial reference of credentials to have clearCredentials() be undefined.

Z-Qi commented 5 years ago

After looking into it further, it seems the issue comes from when the build is run. When index.js is executed, it tries to find '.../misc/credentials', node looks first for credentials.js. For clean installs this means that it will error out with 'module not found' as the file hasn't been generated yet; but with existing builds it will find the old generated credentials.js. This means that the imported reference does not include the new clearCredentials() function (or its export).

The reason it works in subsequent attempts is that after the first error is thrown, the application is rebuilt causing index.js to reimport '.../misc/credentials' which now uses the newly (and correct) generated credentials.js.

The non-code fix for this is to either run npm run createjs to generate the new JS files then npm start as usual. Or to just get the error once and it will work afterwards. The alternative solution would be to modify the start script to generate files before launching the application.

I could implement a 'dirty' fix by importing keytar in index.js but this feels inappropriate.

@lukethompsxn @joel-clarke @AbhinavBehal how do you want to handle this issue?

Z-Qi commented 5 years ago

@lukethompsxn, I've added the keytar fix for now until a start script fix is implemented so that other devs wont need to do any extra 'fixes' on their end.