Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Allow plain objects in Custom loadSession method #126

Merged
merged 4 commits into from
Mar 8, 2021
Merged

Allow plain objects in Custom loadSession method #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
53e9418
Fix custom loadSession method
thecodepixi Mar 5, 2021
5b6381b
Update changelog and docs note
thecodepixi Mar 5, 2021
3bc4d03
Update src/auth/session/storage/custom.ts
thecodepixi Mar 8, 2021
5ae44f8
Move changelog not to Added heading
thecodepixi Mar 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,14 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## Unreleased

### Added
- Allow plain objects to be returned from the `loadCallback` on `CustomSessionStorage` [#126](https://github.com/shopify/shopify-node-api/pull/126)

### Fixed


## [1.1.0] - 2021-03-02

- Minor text/doc changes
- Added `2021-01` API version to enum. [#117](https://github.com/shopify/shopify-node-api/pull/117)
- Allow retrieving offline sessions using `loadCurrentSession`. [#119](https://github.com/shopify/shopify-node-api/pull/119)
Expand Down
3 changes: 2 additions & 1 deletion docs/issues.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ Before you start writing your application, please note that the Shopify library
- Create a new instance of `CustomSessionStorage`, passing in callbacks to store, load, and delete sessions.
- Implement those callbacks so that they perform the necessary actions in your preferred storage method.
- Provide that instance when calling `Shopify.Context.initialize`, for example:

```ts
// src/my_session_storage.ts
import { SessionStorage } from '@shopify/shopify-api';

async function storeCallback(session: Session): Promise<boolean> { ... }
async function loadCallback(id: string): Promise<Session | undefined> { ... }
async function loadCallback(id: string): Promise<Session | Record<string, unknown> | undefined> { ... }
async function deleteCallback(id: string): Promise<boolean> { ... }

const mySessionStorage = new Shopify.Session.CustomSessionStorage(
Expand Down
8 changes: 6 additions & 2 deletions src/auth/session/storage/custom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import * as ShopifyErrors from '../../../error';
export class CustomSessionStorage implements SessionStorage {
constructor(
readonly storeCallback: (session: Session) => Promise<boolean>,
readonly loadCallback: (id: string) => Promise<Session | undefined>,
readonly loadCallback: (id: string) => Promise<Session | Record<string, unknown> | undefined>,
readonly deleteCallback: (id: string) => Promise<boolean>,
) {
this.storeCallback = storeCallback;
Expand All @@ -24,7 +24,7 @@ export class CustomSessionStorage implements SessionStorage {
}

public async loadSession(id: string): Promise<Session | undefined> {
let result: Session | undefined;
let result: Session | Record<string, unknown> | undefined;
try {
result = await this.loadCallback(id);
} catch (error) {
Expand All @@ -35,6 +35,10 @@ export class CustomSessionStorage implements SessionStorage {
if (result) {
if (result instanceof Session) {
return result;
} else if (result instanceof Object && 'id' in result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this but I wonder if it wouldn't be simpler to simply take in the JSON string? We could allow both but that seems a bit much.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I have strong feeling for or against that, it would just require more checking. One thing it does raise in my mind is that we're maybe reaching into their storage implementation by doing that? The custom storage solution should handle its own data in the loadCallback and then give it to us to essentially "pass through" loadSession. By offering to manipulate stringified objects (or potentially non-parseable strings) we're combining their solution with our solution, in a way. But if parsing doesn't work, that's their own error, not ours.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair - at some point we have to translate the external object to ours, but it does make it more flexible if we don't assume it will be JSON.

Even though I suspect JSON will be used in the vast majority of the cases, there's no reason not to support other methods as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's no reason not to support other methods as well.

I can't tell if we're on the same page here? Do you mean we should add support for further data types in this method? My philosophy is "please make your data into this shape first" and have them pass in at least a JSON object. I think the manipulation of their data should be more delegated to their custom callback than to the library, personally. But since the library is what stipulates "make this a Session" it makes sense that we handle that step for them.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I was agreeing with you - as long as they give us an object that maps to what our session looks like, apps can do whatever they want with their objects. By 'other formats' I meant in how they would possibly serialize and de-serialize the object to store it in their database (like say a MongoDB object that's NOT converted to JSON, or something).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok yes! Yeah, I think this way we're being as flexible as possible, because we don't really care what they do to store/access the data, as long as it comes back to us in the right shape.

let session = new Session(result.id as string);
session = {...session, ...result};
return session;
} else {
throw new ShopifyErrors.SessionStorageError(
`Expected return to be instanceof Session, but received instanceof ${result!.constructor.name}.`,
Expand Down