-
Notifications
You must be signed in to change notification settings - Fork 31
feat: adding shopify oxygen server sdk #991
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
base: main
Are you sure you want to change the base?
Conversation
This commit level has a barebones working version of the Oxygen SDK
This is to ensure that we only have 1 poll instance per request
|
@launchdarkly/browser size report |
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
this commit will add in the initial set of oxygen sdk unit test
addressing lint issues introduced by unit tests
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 assume the environment only supports async crypto?
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.
Ah I need to revisit this, I copied the implementation form the Akamai Edge sdk to get something working... but forgot to revisit. Will look into what is provided by https://shopify.dev/docs/storefronts/headless/hydrogen/deployments/oxygen-runtime#web-crypto-api
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.
ah but yes, the environment only supports async crypto.
| const result = this._cryptoJSHmac.finalize(); | ||
|
|
||
| if (encoding === 'base64') { | ||
| return result.toString(CryptoJS.enc.Base64); |
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 this import missing?
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.
So cryptojs actually puts itself into global scope https://github.com/brix/crypto-js/blob/develop/src/core.js
| import type { Info, PlatformData, SdkData } from '@launchdarkly/js-server-sdk-common'; | ||
|
|
||
| // TODO: maybe not the right name for this package? Currently copied from Akamai... I think | ||
| const name = '@launchdarkly/shopify-oxygen-server-sdk'; |
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.
It reasonably matches our naming convention for these. It runs on "shopify-oxygen" and is a "server-sdk".
|
|
||
| export default class OxygenRequests implements platform.Requests { | ||
| // @ts-ignore - Cache API is available in Shopify Oxygen runtime | ||
| private _cache: Cache | null = null; |
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.
Could we have like a globals.d.ts or something that would make ts aware of the shape of these?
| ...options, | ||
| }); | ||
|
|
||
| const cachedResponse = await cache.match(request); |
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 to be looking at a TTL on these?
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.
Or it is just handled by the cache-control?
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.
cache control handles these and will return null on expired extries.
Additionally changed the way we pull in crypto-js so it would work with module syntax
Requirements
Related issues
Provide links to any issues in this repository or elsewhere relating to this pull request.
Describe the solution you've provided
Provide a clear and concise description of what you expect to happen.
Describe alternatives you've considered
Provide a clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context about the pull request here.