Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add code documentation to new extension #46

Open
wants to merge 8 commits into
base: extension-as-remote
Choose a base branch
from

Conversation

rafael-mindreau
Copy link
Collaborator

Please comment on parts that are still not clear or need more clarification.

@pheyvaer
Copy link
Contributor

pheyvaer commented Aug 29, 2023

I fails to show the login widget in the showcase for me. I see in the console 10.000s of messages that say

Extension got disconnected, replenishing connections...

I'm using Chrome v116 and Node v18.16.1.

Also, I added a Markdown linter to format the README.

@rafael-mindreau
Copy link
Collaborator Author

I fails to show the login widget in the showcase for me. I see in the console 10.000s of messages that say

Extension got disconnected, replenishing connections...

I'm using Chrome v116 and Node v18.16.1.

Also, I added a Markdown linter to format the README.

Thanks for adding that linter!

I'm not able to replicate exactly your issue, if you try to open the showcase app when the extension is not installed, it will throw some errors.

Also, if the extension has a general failure, the showcase was not really designed to catch those errors yet, so it might infinitely try to reach the mothership. These issues are often fixed by refreshing the extension (pressing update in the extensions page):

image

Please let me know if this issue persists.

@pheyvaer
Copy link
Contributor

It still doesn't work.

@rafael-mindreau
Copy link
Collaborator Author

It still doesn't work.

Are you trying to log in with an identity created on solid playground which no longer exists? (created yesterday)

@pheyvaer
Copy link
Contributor

It still doesn't work.

Are you trying to log in with an identity created on solid playground which no longer exists? (created yesterday)

No, I'm using my own WebID and pod.

@pheyvaer
Copy link
Contributor

@rafael-mindreau I got it working. The EXTENSION_ID in identityPlugin.js didn't match the ID that my Chrome instance gave the extension. Should it have?

@rafael-mindreau
Copy link
Collaborator Author

@rafael-mindreau I got it working. The EXTENSION_ID in identityPlugin.js didn't match the ID that my Chrome instance gave the extension. Should it have?

Oh WOW! Good that you mention that, because I've totally forgotten about this. When I added this into the code, I suspected that this might not be static. I had an existential crisis trying to replicate this issue, so I'm glad you found it. I will add this into documentation!

Copy link
Contributor

@pheyvaer pheyvaer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the ESLint settings. Can you make sure that there are no errors/warnings anymore? That way my review will go smoother. Thanks!

I also added some initial comments.

* Queries for an IDP for a given WebID
* We use the IDPs generally for redirecting the user to the login/authorization flow of their IDP
* TODO: If your WebID does not exist or the IDP cannot be determined, this will fail with an error and no fallback
*/
async _getIDPsFromWebID(webId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should happen by the extension. That way the extension always returns the IDP. This is not security concern, because this data is public in any case and there is not authentication happening at this point.

@@ -28,10 +37,15 @@ export default class IdentityWidget {
console.log('%cDISCONNECT', 'padding: 5px; border-radius: 3px; background: #ff3333; font-weight: bold; color: white', 'Extension got disconnected, replenishing connections...');
}

// TODO: Long-lived connections will shut down after a longer period of inactivity. They are hydrated by opening the connection again after it is closed
// The idea is to only update the connection when it is absolutely needed
// Keep-alive mechanism TODO: check if there isn't a better way of handling this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every TODO should have a issue linked to it.

showcase-app/plugin/identityPlugin.js Show resolved Hide resolved
- fix npm vulnerability
- add documentation
- pin extension id
@pheyvaer pheyvaer self-requested a review November 2, 2023 10:06
.prettierrc Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
showcase-app/main.js Outdated Show resolved Hide resolved
showcase-app/main.js Outdated Show resolved Hide resolved
showcase-app/plugin/identityPlugin.js Outdated Show resolved Hide resolved
showcase-app/plugin/identityPlugin.js Outdated Show resolved Hide resolved
showcase-app/plugin/identityPlugin.js Outdated Show resolved Hide resolved
.eslintrc.cjs Outdated Show resolved Hide resolved
@svdwyer svdwyer requested a review from pheyvaer November 14, 2023 08:57
@pheyvaer
Copy link
Contributor

Instead of merging, you need to push this code to main of https://github.com/SolidLabResearch/solid-identity-manager

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants