Skip to content
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

Add support for Edge Runtime #110

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Add support for Edge Runtime #110

merged 2 commits into from
Sep 13, 2023

Conversation

jhamon
Copy link
Collaborator

@jhamon jhamon commented Sep 13, 2023

Problem

People attempting to use this library in Vercel's Edge Runtime are hitting runtime exceptions due to reliance on some node utilities that are not available.

This addresses issue #108

Solution

  • Setup new npm run test:integration:edge which runs integration tests using a special jest context designed to highlight when code is using dependencies that are not available on edge.
  • Integrate this test run into CI

Code changes to get these tests passing:

  • Add an isEdge() utility that looks for a global var, EdgeRuntime to detect whether execution is happening in the Edge Runtime.
  • Use that util to guard things which won't work in Edge. Things not supported in Edge:
    • Pulling os info for the user-agent string. This info can just be omitted from the user-agent.
    • Using fs to read package.json version, also for the user-agent string. The version number is the most important thing in the user-agent so I do need some other way of getting that info. Importing json as a module is possible in es6, but I was getting typescript build errors when trying to grab package.json directly because that file is outside of the tsconfig rootDir. So, I adjusted the publish steps to dump version info to a place it can be safely referenced, src/version.json.
    • Runtime validation using AJV. The code generation that happens in the compile step causes Edge to blow up. So we skip runtime validations for Edge for now. If they are developing with TypeScript, these should be largely unnecessary anyway.

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Infrastructure change (CI configs, etc)

Test Plan

  • New CI job for edge integration tests should be green.
  • For changes to publish jobs, I did some local testing to explore what the npm version command does and test the jq commands. But won't know for sure that this is all correct, though, until trying to ship a release.

shell: bash
run: npm version ${{ inputs.releaseType }} -m '[skip ci] Publish release %s'
run: npm version ${{ inputs.releaseType }} --no-git-tag-version
- name: 'Commit and tag version changes'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These changes plus the addition of the version script in the package.json file are needed to update src/version.json with the new version number prior to the commit/tag. Even though the npm version command automatically invokes the npm run version script for us, it doesn't happen until after the automatic commit. So we need to tell npm not to do the auto commit with the --no-git-tag-version flag and handle it ourselves.

@@ -35,6 +35,7 @@ jobs:
jq --arg newVersion "$devVersion" '.version = $newVersion' package.json > package.tmp && mv package.tmp package.json
jq --arg newVersion "$devVersion" '.version = $newVersion' package-lock.json > package-lock.tmp && mv package-lock.tmp package-lock.json
jq --arg newVersion "$devVersion" '.packages[""].version = $newVersion' package-lock.json > package-lock.tmp && mv package-lock.tmp package-lock.json
jq --null-input --arg version "$devVersion" '{"name": "@pinecone-database/pinecone", "version": $version}' > src/version.json
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For dev builds, we never relied on the npm version command to set version numbers. So we update src/version.json a little differently also. I tested this jq command on my local machine.

@@ -224,6 +225,11 @@ export const errorFormatter = (subject: string, errors: Array<ErrorObject>) => {
};

export const buildValidator = (errorMessagePrefix: string, schema: any) => {
if (isEdge()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This turns runtime validation into a noop in the Edge Runtime.

const packageJson = JSON.parse(fs.readFileSync('./package.json', 'utf8'));
import { getOSInfo } from './os';
import { isEdge } from './environment';
import * as packageInfo from '../version.json';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We removed problematic imports for Edge here.

@jhamon jhamon marked this pull request as ready for review September 13, 2023 16:32
@jhamon jhamon force-pushed the jhamon/edge-runtime-support branch from 1a01eba to b13eb8c Compare September 13, 2023 17:10
@jhamon jhamon requested a review from rohanshah18 September 13, 2023 17:11
Copy link
Contributor

@austin-denoble austin-denoble left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for getting on top of this so quickly. One callout in the /npm-release/action.yml file that I think needs updating.

shell: bash
run: npm version ${{ inputs.releaseType }} -m '[skip ci] Publish release %s'
run: npm version ${{ inputs.releaseType }} --no-git-tag-version
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need run here?

Suggested change
run: npm version ${{ inputs.releaseType }} --no-git-tag-version
run: npm run version ${{ inputs.releaseType }} --no-git-tag-version

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is actually a command called npm version. It knows how to bump versions and whatnot and commit those changes (unless you disabled that with a flag as we have here).

As part of that command run, it executes certain lifecycle scripts. So if you have defined a version command in the scripts section of your package.json, it will automatically be invoked. There's also a preversion hook. But you don't need to call npm run version yourself. I agree it's confusing that the command name and lifecycle script hooks have the same name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good to know. Thank you!

@jhamon jhamon merged commit 3671748 into main Sep 13, 2023
@jhamon jhamon deleted the jhamon/edge-runtime-support branch September 13, 2023 19:05
jhamon added a commit that referenced this pull request Sep 14, 2023
## Problem

PR #110 resolved the problem with `fs` imports that was happening in the
Edge Runtime, but feedback on a dev build shows that `os` imports are
still an issue even though the `@edge-runtime/jest-environment` test
build is passing.

This relates to:
- #108 

## Solution

For now, the easiest thing is to just remove references to `os`. The
information it adds to the user-agent header is nice to have, but not
essential.

## Type of Change

- [x] Bug fix (non-breaking change which fixes an issue)
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.

2 participants