-
Notifications
You must be signed in to change notification settings - Fork 401
docs: snippet for pubnub apis. #458
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
Conversation
…pp-context, mobile-push, access-manager, channel-groups.
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
…before snippet compilation happens
…od on subscriptionSet entity
parfeon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In each file (and sometimes even within the same file) there is a different indention level (or different tab width). Our codebase uses 2 spaces for tab.
I'm leaving early review comments because new files appeared while I was making this one and probably have similar issues + my browser lags with this much change suggestions.
src/core/pubnub-common.ts
Outdated
| */ | ||
| public grantToken(parameters: PAM.GrantTokenParameters, callback: ResultCallback<PAM.GrantTokenResponse>): void; | ||
| public grantToken( | ||
| parameters: PAM.GrantTokenParameters | PAM.ObjectsGrantTokenParameters, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to have ObjectsGrantTokenParameters? Probably it has been added because I forgot to mark it as deprecated since it uses spaces and users while the new interface uses uuids and confusingly channels (which both regular and App Context metadata).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
| "compilerOptions": { | ||
| "module": "esnext", | ||
| "target": "es2017", | ||
| "outDir": "./docs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be careful here because during the release process run all changes added to the git. Should we add the docs folder to the .gitignore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since
"noEmit": true,
It won't generate any js files on tsc.
docs-snippets/access-manager.ts
Outdated
| } catch (status) { | ||
| console.log(status); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We receive error in catch blocks, not status. Though this one is PubNubError which has a status field with State object in it with additional details, so maybe something like this:
| } catch (status) { | |
| console.log(status); | |
| } | |
| } catch (error) { | |
| console.error(`Grant token error: ${error}.${ | |
| error.status ? ` Additional information: ${error.status}` : '' }`); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all error handling updated as per suggestion.
docs-snippets/access-manager.ts
Outdated
| } catch (status) { | ||
| console.log(status); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We receive error in catch blocks, not status. Though this one is PubNubError which has a status field with State object in it with additional details, so maybe something like this:
| } catch (status) { | |
| console.log(status); | |
| } | |
| } catch (error) { | |
| console.error(`Grant token error: ${error}.${ | |
| error.status ? ` Additional information: ${error.status}` : '' }`); | |
| } |
docs-snippets/access-manager.ts
Outdated
| } catch (status) { | ||
| console.log(status); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We receive error in catch blocks, not status. Though this one is PubNubError which has a status field with State object in it with additional details, so maybe something like this:
| } catch (status) { | |
| console.log(status); | |
| } | |
| } catch (error) { | |
| console.error(`Grant token error: ${error}.${ | |
| error.status ? ` Additional information: ${error.status}` : '' }`); | |
| } |
docs-snippets/publish-subscribe.ts
Outdated
| } catch (status) { | ||
| console.log(`publishing failed with status: ${status}`); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We receive error in catch blocks, not status. Though this one is PubNubError which has a status field with State object in it with additional details, so maybe something like this:
| } catch (status) { | |
| console.log(`publishing failed with status: ${status}`); | |
| } | |
| } catch (error) { | |
| console.error(`Message publish error: ${error}.${ | |
| error.status ? ` Additional information: ${error.status}` : '' }`); | |
| } |
docs-snippets/app-context.ts
Outdated
| } catch (error) { | ||
| console.error(error); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PubNubError has a status field with State object in it with additional details, so maybe something like this:
| } catch (error) { | |
| console.error(error); | |
| } | |
| } catch (error) { | |
| console.error(`Set channel metadata error: ${error}.${ | |
| error.status ? ` Additional information: ${error.status}` : '' }`); | |
| } |
| // Using UUID from the config - default when uuid is not passed in the method | ||
| try { | ||
| const response = await pubnub.objects.getUUIDMetadata(); | ||
| console.log(`getUUIDMetadata response: ${response}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will give output like this:
getUUIDMetadata response: [object Object]
not really informative.
But it could be better this way:
| console.log(`getUUIDMetadata response: ${response}`); | |
| console.log('getUUIDMetadata response:', response); |
| const response = await pubnub.objects.getAllChannelMetadata({ | ||
| filter: 'name LIKE "*Team"', | ||
| }); | ||
| console.log(`getAllChannelMetadata response: ${response}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will give output like this:
getAllChannelMetadata response: [object Object]
not really informative.
But it could be better this way:
| console.log(`getAllChannelMetadata response: ${response}`); | |
| console.log('getAllChannelMetadata response:', response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All response output updated.
| const response = await pubnub.objects.setMemberships({ | ||
| uuid: "my-uuid", | ||
| channels: [ | ||
| "my-channel", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the formatter really align this line that far and not four spaces from the channels work start on the previous line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied formatting to all files.
Not sure why, I see sometimes git doesn't show the same indentation as I see in my IDE!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configuration is 2 spaces Tab.
…tax support, Error handling refactor in code snippets
* Indentation as per other code base
| const decrypted = pubnub.decrypt(encrypted); // Pass the encrypted data as the first parameter in decrypt Method | ||
| // snippet.end | ||
|
|
||
| // snippet.decryptFileBasicUsage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // snippet.decryptFileBasicUsage | |
| // snippet.decryptFileBasicUsage | |
| // Node.js example | |
| // import fs from 'fs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // snippet.end | ||
|
|
||
| // snippet.setProxyBasicUsage | ||
| pubnub.setProxy({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention here that this is only Node.js stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioned in docs already here.
Still no harm adding into example again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comment.
|
|
||
| // snippet.addDeciveToChannelBasicUsage | ||
| // Function to add a device to a channel for APNs2 | ||
| async function addDeviceToChannelAPNs2() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we don't need functions (there were few before this file as well).
| console.log('Operation done for APNs2!'); | ||
| console.log('Response:', result); | ||
| } catch (error) { | ||
| console.log('Operation failed with error for APNs2:', error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to have similar output as in snippets above?
In previous review summary, I've left, that because new files appeared I've stopped. Same issues as I marked in previous files were in the rest (status in catch block instead of error and more detailed output).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scanning files which did not have comments earlier ⏳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| console.log(response); | ||
| }) | ||
| .catch((error) => { | ||
| console.log(error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want more detailed output here like this:
| console.log(error); | |
| console.error( | |
| `State set failed: ${error}.${ | |
| (error as PubNubError).status ? ` Additional information: ${(error as PubNubError).status}` : '' | |
| }`, | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated in presence.ts
docs-snippets/publish-subscribe.ts
Outdated
| ); | ||
| } | ||
| // snippet.end | ||
| // *********** Compilation Error due to wrong code ***************** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this one in final document?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
docs-snippets/presence.ts
Outdated
| console.log(`hereNow response: ${response}`); | ||
| } catch (status) { | ||
| console.log(`hereNow failed with error: ${status}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issues in this file as in previous with how object will be printed in formatted string and status instead of error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
covered in presence.ts file
docs-snippets/message-persistence.ts
Outdated
| console.log(`fetch messages response: ${response}`); | ||
| } catch (status) { | ||
| console.log(`fetch messages failed with error: ${status}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issues in this file as in previous with how object will be printed in formatted string and status instead of error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message-persistence.ts updated
docs-snippets/getting-started.ts
Outdated
| console.log(`Message published with timetoken: ${result.timetoken}`); | ||
| console.log(`You: ${text}`); | ||
| } catch (error) { | ||
| console.error(`Publish failed: ${error}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More detailed output with status?
docs-snippets/getting-started.ts
Outdated
| if (event.category === 'PNConnectedCategory') { | ||
| console.log('Connected to PubNub chat!'); | ||
| } else if (event.category === 'PNNetworkIssuesCategory') { | ||
| console.log('Connection lost. Attempting to reconnect...'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one depending on from settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description updated
parfeon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small note, but in general looks good! :)
| console.log(`\nMessage sent successfully!`); | ||
| } catch (error) { | ||
| // Handle publish errors | ||
| console.error(`\n❌ Failed to send message: ${error}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can add more info :)
|
@pubnub-release-bot release |
|
🚀 Release successfully completed 🚀 |
refactor: removed deprecation warning from deleteMessages method.
Removed deprecation warning from deleteMessages method.
docs: code snippets for using pubnub apis.
Added code snippets for docs.