Skip to content

Conversation

@monsagri
Copy link
Contributor

@monsagri monsagri commented Apr 12, 2023

[sc-194414]

This adds an Edge SDK for Vercel to be used with Vercel Edge Configs.

I've added some basic testing, and using the built package in a vercel project using npm link works as expected.

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #194414: Update the SDK to pull in config from Vercel.

Comment on lines 11 to 12
// TODO: Investigate if we can actually send events
sendEvents: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving this for a follow up PR as it's a nice to have

Copy link
Member

Choose a reason for hiding this comment

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

If vercel has fetch, then it should be possible. Not sure on the value, because I have not really been in the loop how we use these edge SDKs and why it is fine for them to not send events.

Copy link
Contributor

Choose a reason for hiding this comment

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

I left a slack comment for @yusinto in regards to Cloudflare. When we go to enable events we want to also add in the track call. I'm not sure if it's waiting until sendEvents is usable to add that in.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we probably wouldn't want to add it before it actually sends events.

@monsagri monsagri marked this pull request as ready for review April 13, 2023 14:22
@kinyoklion
Copy link
Member

We figured we would need a shared folder for some potentially common edge provider items, but it is hard to say for sure until you make 2. :-)

@yusinto
Copy link
Contributor

yusinto commented Apr 18, 2023

Please take a look at #83 which has the common code for cloudflare and vercel refactored to packages/shared/sdk-server-edge.

@monsagri
Copy link
Contributor Author

Please take a look at #83 which has the common code for cloudflare and vercel refactored to packages/shared/sdk-server-edge.

Got it thanks! Actually had a scroll through already and will be making sure to get this updated today/tomorrow latest

Thanks for doing all the heavy lifting with the shared stuff!

@yusinto
Copy link
Contributor

yusinto commented Apr 18, 2023

Thanks for doing all the heavy lifting with the shared stuff!

No worries! Thank you for doing the Vercel sdk. I have merged #83 now so you'll be able to use it from main which makes it easier.

@yusinto
Copy link
Contributor

yusinto commented Apr 19, 2023

Please take a look at #87 for producing cjs and esm bundles for the cloudflare sdk which you will need for Vercel also. TLDR your build command should use scripts/build-package.sh which will do the job. The relevant code are:

  "type": "module",
  "exports": {
    "require": "./dist/cjs/src/index.js",
    "import": "./dist/esm/src/index.js"
  },
  "main": "./dist/cjs/src/index.js",
  "types": "./dist/cjs/src/index.d.ts",
  "files": [
    "/dist"
  ],
  "scripts": {
    "build": "../../../scripts/build-package.sh",

@yusinto
Copy link
Contributor

yusinto commented Apr 20, 2023

The Cloudflare SDK 0.0.1 has been published. All code are in main now so you can safely follow the cloudflare sdk for vercel. Let me know if everything makes sense. Good luck!

@monsagri monsagri requested a review from kinyoklion April 21, 2023 11:31
@monsagri
Copy link
Contributor Author

I've made the changes that we talked about and adapted everything to use the new common modules where possible.
Missing some tests against the main implementation for now using something like miniflare to mock the edge config, but think that can come as a follow up?

Copy link
Contributor

@yusinto yusinto left a comment

Choose a reason for hiding this comment

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

Nice work! I requested some changes but nothing serious.

You will also need to modify release-please-config.json for the initial publishing of this package. See this.

@monsagri monsagri requested a review from yusinto April 24, 2023 11:24
@monsagri monsagri merged commit 5f5eda3 into main Apr 25, 2023
@monsagri monsagri deleted the ffeldberg/sc-194414/update-the-sdk-to-pull-in-config-from-vercel branch April 25, 2023 11:27
@github-actions github-actions bot mentioned this pull request Apr 25, 2023
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.

5 participants