-
Notifications
You must be signed in to change notification settings - Fork 31
fix: Update to major version of @vercel/edge-config #393
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
fix: Update to major version of @vercel/edge-config #393
Conversation
| "packages/sdk/vercel/examples/complete", | ||
| "packages/sdk/vercel/examples/route-handler", |
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.
Include examples in the workspace to take advantage of the monorepo setup.
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.
No functional changes here just minor refactoring for the sake of my ocd.
| "@launchdarkly/vercel-server-sdk": "1.3.0", | ||
| "@vercel/edge-config": "^1.1.0", |
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.
These are the main updates.
|
This pull request has been linked to Shortcut Story #235330: Major update for vercel edge config. |
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.
appDir is now part of @vercel/edge-config and no longer experimental. Hence this file is unnecessary.
| "@launchdarkly/vercel-server-sdk": "1.3.0", | ||
| "@vercel/edge-config": "^1.1.0", | ||
| "next": "14.1.2", |
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.
Major updates here too.
| "test": "NODE_OPTIONS=\"--experimental-vm-modules --no-warnings\" jest --ci --runInBand", | ||
| "coverage": "yarn test --coverage", | ||
| "check": "yarn prettier && yarn lint && yarn build && yarn test && yarn doc" | ||
| "check": "yarn prettier && yarn lint && yarn build && yarn test" |
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.
doc never exists in this project, so this was breaking.
| token: '', | ||
| version: '', | ||
| type: 'vercel', | ||
| }, |
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 newly added in the major version of edge-config.
| !process.env.LD_CLIENT_SIDE_ID || | ||
| !parseConnectionString(process.env.EDGE_CONFIG) | ||
| ) { | ||
| return NextResponse.rewrite(new URL('/missing-edge-config', request.url)); |
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 not sure how this example could have worked? request does not exist.
| false, | ||
| ); | ||
|
|
||
| return hotDogFaviconEnabled |
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 applies to request here as above.
|
If you want to release this, versus bundling it with other changes in the future, then don't use chore. If we want a releasable unit that isn't |
|
This should be a feat, because it breaks the previous version types. |
A customer will need to update their code with this upgrade? If so |
|
@kinyoklion I changed this to fix because I tested this PR and it does not break existing usage. If anything, it fixes the EdgeConfigClient type mismatch in |
|
I know it isn't related to this PR, but is detox e2e unstable? Because of how our releases work we need to be extra careful that CI jobs are stable. |
It used to be stable but it has declined to be not stable. It's very disappointing but I may have to disable/exclude it completely, or else only run it when RN changes are made. I'll create a separate PR to fix this. |
|
The detox tests are definitely flaky in CI although they are stable locally when run through |
🤖 I have created a release *beep* *boop* --- <details><summary>vercel-server-sdk: 1.3.1</summary> ## [1.3.1](vercel-server-sdk-v1.3.0...vercel-server-sdk-v1.3.1) (2024-03-14) ### Bug Fixes * Update to major version of @vercel/edge-config ([#393](#393)) ([bbaf01c](bbaf01c)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
We updated @vercel/edge-config package to use the major version in this [pr](#393) however I overlooked updating the corresponding example apps to use the workspace versions of our vercel sdk.
Fixes #379 . I have tested this locally and verified the type errors are fixed and the example application (complete) still works.