-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: Add Fastly Edge SDK #723
base: main
Are you sure you want to change the base?
Conversation
@launchdarkly/js-sdk-common size report |
@launchdarkly/js-client-sdk size report |
@launchdarkly/js-client-sdk-common size report |
@@ -0,0 +1,46 @@ | |||
import { internal } from '@launchdarkly/js-server-sdk-common'; |
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 tests in this directory were copied over from @launchdarkly/sdk-server-edge
@@ -0,0 +1,59 @@ | |||
import { PlatformData, SdkData } from '@launchdarkly/js-server-sdk-common'; |
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 was copied from @launchdarkly/sdk-server-edge
(docs ticket is at DOCS-973 and currently scheduled for week of 1/6) |
const platformData = createPlatformInfo(); | ||
|
||
expect(platformData.platformData()).toEqual({ | ||
name: 'Fastly Edge', |
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'm thinking about changing this to Fastly Compute based on Fastly's branding here. What do you think, @kinyoklion?
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.
That works for me.
"dist" | ||
], | ||
"scripts": { | ||
"build": "tsup && ../../../scripts/replace-version.sh .", |
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.
@kinyoklion, let me know if there is a better way to get the version in src.createPlatformInfo.ts
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.
Yes, there is a better way.
"packages/sdk/cloudflare": {
"extra-files": [
{
"type": "json",
"path": "jsr.json",
"jsonpath": "$.version"
},
{
"type": "json",
"path": "example/package.json",
"jsonpath": "$.dependencies['@launchdarkly/cloudflare-server-sdk']"
},
"src/createPlatformInfo.ts"
]
},
Here we can see how to not use "workspace" in the example project (so customers can use it outside the repo), as well as update the createPlatformInfo
file.
@@ -0,0 +1,116 @@ | |||
import type { |
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 was copied from @launchdarkly/sdk-server-edge
@@ -0,0 +1,24 @@ | |||
import { Info, internal, LDClientImpl, LDOptions } from '@launchdarkly/js-server-sdk-common'; |
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 was copied from @launchdarkly/sdk-server-edge
@@ -0,0 +1,9 @@ | |||
const createCallbacks = () => ({ |
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 is the new empty implementation to get around the lack of node:events
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 I think we can make a task, not blocking this, which removes the dependency from the common package.
That would then allow for the eventually removal of most of the code for this package.
@@ -0,0 +1,17 @@ | |||
import { BasicLogger, LDOptions } from '@launchdarkly/js-server-sdk-common'; |
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 was copied from @launchdarkly/sdk-server-edge
sdkKey, | ||
createPlatformInfo(), | ||
finalOptions, | ||
eventsBackendName || DEFAULT_EVENTS_BACKEND_NAME, |
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 chose to pass eventsBackendName
as a new parameter instead of changing the options
type used by LDClient. Let me know if you think this is the wrong approach.
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.
The LDClient implementation is your implementation, so it could take the fastly options without any modification to the base options. You would just need to move how you destructure it.
That said I am indifferent to it being its own parameter.
@@ -0,0 +1,49 @@ | |||
import CryptoJS from 'crypto-js'; |
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.
Everything in this directory was copied from @launchdarkly/sdk-server-edge
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 know if we need crypto-js, or does fastly compute have its own suitable alternatives?
|
||
fetch(url: string, options: Options = {}): Promise<Response> { | ||
// @ts-ignore | ||
return fetch(url, { ...options, backend: this.eventsBackend }); |
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 is the main differentiator between the other edge SDKs. We are required to pass a fastly-specific backend
parameter.
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've not looked through everything yet, but I did do an initial pass with a few items. Sorry for the delay.
@@ -28,6 +28,15 @@ | |||
"tag-prefix": "cloudflare-server-sdk-" | |||
} | |||
}, | |||
"Fastly": { |
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 am not sure about this key being capitalized.
const platformData = createPlatformInfo(); | ||
|
||
expect(platformData.platformData()).toEqual({ | ||
name: 'Fastly Edge', |
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.
That works for me.
@@ -0,0 +1,9 @@ | |||
const createCallbacks = () => ({ |
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 I think we can make a task, not blocking this, which removes the dependency from the common package.
That would then allow for the eventually removal of most of the code for this package.
sdkKey, | ||
createPlatformInfo(), | ||
finalOptions, | ||
eventsBackendName || DEFAULT_EVENTS_BACKEND_NAME, |
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.
The LDClient implementation is your implementation, so it could take the fastly options without any modification to the base options. You would just need to move how you destructure it.
That said I am indifferent to it being its own parameter.
@@ -0,0 +1,49 @@ | |||
import CryptoJS from 'crypto-js'; |
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 know if we need crypto-js, or does fastly compute have its own suitable alternatives?
}, | ||
"dependencies": { | ||
"@fastly/js-compute": "^3.28.0", | ||
"@launchdarkly/fastly-server-sdk": "workspace:^" |
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 may want to consider using the version number here and having release-please update it. That makes it more like a real example.
"extra-files": [
{
"type": "json",
"path": "example/package.json",
"jsonpath": "$.dependencies['@launchdarkly/cloudflare-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.
For example then the user can copy it out of the repo and it should work. The yarn workspace will link it locally during development.
No problem and I appreciate your feedback. I'll take another pass next week. |
Requirements
Describe the solution you've provided
This PR adds a new Fastly Compute SDK. This SDK must be used in conjunction with our upcoming Fastly KV integration. The SDK is essentially the same as the other edge integrations (Akamai, Cloudflare, Vercel), with the following changes:
node:events
is not compatible with Fastly's runtime. As a result, we cannot use@launchdarkly/sdk-server-edge
. Instead, I copied the contents of@launchdarkly/sdk-server-edge
into the fastly package and replacedcreateCallbacks.ts
with an empty implementation.sendEvents
by default.eventsBackendName
option. A Fastly Backend configured tohttps://events.launchdarkly.com
is required for sending events. The default value islaunchdarkly
. This option is passed to Fastly's customized fetch().I added an example app that demonstrates using the SDK to evaluate a feature flag edge to control the static image served.
I published an alpha version to npm here.