Skip to content

feat(sdk): add x-pd-sdk-version, triggerDeploy (1.0.9) #14836

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Dec 5, 2024
Merged

feat(sdk): add x-pd-sdk-version, triggerDeploy (1.0.9) #14836

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3f02372
feat(sdk): add x-pd-sdk-version header, etc
tjk Dec 4, 2024
039eb96
add server example
tjk Dec 4, 2024
5cc123b
rewrite file entirely
tjk Dec 4, 2024
dfad322
Revert "rewrite file entirely"
tjk Dec 4, 2024
29ec2ae
import check for shared/browser
tjk Dec 4, 2024
2663c9a
add triggerDeploy
tjk Dec 4, 2024
facbdcd
various improvements (and fix test)
tjk Dec 4, 2024
4a42019
Merge branch 'master' into add-sdk-version
tjk Dec 4, 2024
c204d9d
get the ts extension working
tjk Dec 4, 2024
24473f9
fix lint errors
tjk Dec 4, 2024
e8e44f0
various fixes
tjk Dec 4, 2024
ea290cb
try to fix stupid jest .js load
tjk Dec 4, 2024
801c5aa
please
tjk Dec 4, 2024
43bd76b
please
tjk Dec 4, 2024
19bf059
please
tjk Dec 4, 2024
cdf99e2
maybe??
tjk Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/sdk/examples/browser/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
default:
make -j2 server delayed-open
Comment on lines +1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove parallel execution to prevent race conditions

The -j2 flag runs targets in parallel which could cause the browser to open before the server is ready. The server needs to be fully started before opening the browser.

default:
-	make -j2 server delayed-open
+	make server & make delayed-open

Committable suggestion skipped: line range outside the PR's diff.


server:
cd ../.. && python -m http.server
Comment on lines +4 to +5
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance server security and configuration

The current server implementation has several issues:

  1. Serves the entire parent directory, potentially exposing sensitive files
  2. No port configuration
  3. No graceful shutdown mechanism
+PORT ?= 8000
+
server:
-	cd ../.. && python -m http.server
+	cd examples/browser && python -m http.server $(PORT)

Also consider adding a target for stopping the server:

.PHONY: stop-server
stop-server:
	lsof -ti:$(PORT) | xargs kill -9 2>/dev/null || true


delayed-open:
sleep 1 && xdg-open http://localhost:8000/examples/browser/index.html
Comment on lines +7 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve cross-platform compatibility and error handling

The current implementation has platform-specific issues:

  1. xdg-open is Linux-specific
  2. Hard-coded port number
  3. Fixed delay might not be sufficient on slower systems
+PORT ?= 8000
+DELAY ?= 1
+
+ifeq ($(OS),Windows_NT)
+  OPENER := start
+else
+  UNAME := $(shell uname -s)
+  ifeq ($(UNAME),Darwin)
+    OPENER := open
+  else
+    OPENER := xdg-open
+  endif
+endif
+
delayed-open:
-	sleep 1 && xdg-open http://localhost:8000/examples/browser/index.html
+	sleep $(DELAY) && $(OPENER) http://localhost:$(PORT)/examples/browser/index.html || echo "Failed to open browser"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
delayed-open:
sleep 1 && xdg-open http://localhost:8000/examples/browser/index.html
PORT ?= 8000
DELAY ?= 1
ifeq ($(OS),Windows_NT)
OPENER := start
else
UNAME := $(shell uname -s)
ifeq ($(UNAME),Darwin)
OPENER := open
else
OPENER := xdg-open
endif
endif
delayed-open:
sleep $(DELAY) && $(OPENER) http://localhost:$(PORT)/examples/browser/index.html || echo "Failed to open browser"

20 changes: 20 additions & 0 deletions packages/sdk/examples/browser/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<!DOCTYPE html>
<html>
<body>
<h1>Load SDK in browser</h1>
<script type="importmap">
{
"imports": {
"@rails/actioncable": "/node_modules/@rails/actioncable/app/assets/javascripts/actioncable.esm.js"
}
}
</script>
<script type="module">
import { createFrontendClient } from "/dist/browser/browser/index.js"
const client = createFrontendClient()
const p = document.createElement("p")
p.textContent = "sdk version: " + client.version
document.body.appendChild(p)
</script>
</body>
</html>
2 changes: 2 additions & 0 deletions packages/sdk/examples/server/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
default:
@node index.mjs
10 changes: 10 additions & 0 deletions packages/sdk/examples/server/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { createBackendClient } from "../../dist/server/server/index.js";

const client = createBackendClient({
environment: "development",
credentials: {
clientId: "not-empty",
clientSecret: "not-empty",
},
});
console.log("sdk version: " + client.version);
Comment on lines +1 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding error handling and API example

The example could be more comprehensive by demonstrating error handling and an actual API call to show the x-pd-sdk-version header in action.

Here's a suggested enhancement:

 import { createBackendClient } from "../../dist/server/server/index.js";

+async function main() {
+  try {
     const client = createBackendClient({
       environment: "development",
       credentials: {
         clientId: "not-empty",
         clientSecret: "not-empty",
       },
     });
     console.log("sdk version: " + client.version);
+    
+    // Example API call to demonstrate x-pd-sdk-version header
+    const response = await client.makeRequest();
+    console.log("API Response:", response);
+  } catch (error) {
+    console.error("Error:", error.message);
+    process.exit(1);
+  }
+}
+
+main();

Committable suggestion skipped: line range outside the PR's diff.

5 changes: 3 additions & 2 deletions packages/sdk/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@
"access": "public"
},
"scripts": {
"prepublish": "rm -rf dist && pnpm run build",
"build": "pnpm run build:node && pnpm run build:browser",
"prepublish": "pnpm run build",
"prebuild": "node scripts/updateVersion.mjs",
"build": "rm -rf dist && pnpm run prebuild && pnpm run build:node && pnpm run build:browser",
"build:node": "tsc -p tsconfig.node.json",
"build:browser": "tsc -p tsconfig.browser.json",
"test": "jest",
Expand Down
13 changes: 13 additions & 0 deletions packages/sdk/scripts/updateVersion.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import fs from "fs";
import cp from "child_process";

if (!process.env.CI) {
// make sure people building locally automatically do not track changes to version file
cp.execSync("git update-index --skip-worktree src/version.ts");
}
Comment on lines +4 to +7
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding error handling for git command.

The git command execution should be wrapped in a try-catch block to handle potential errors gracefully, especially if git is not installed or the repository is not initialized.

 if (!process.env.CI) {
   // make sure people building locally automatically do not track changes to version file
-  cp.execSync("git update-index --skip-worktree src/version.ts");
+  try {
+    cp.execSync("git update-index --skip-worktree src/version.ts");
+  } catch (error) {
+    console.warn("Warning: Could not update git index. This may be due to git not being installed or repository not being initialized.");
+  }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!process.env.CI) {
// make sure people building locally automatically do not track changes to version file
cp.execSync("git update-index --skip-worktree src/version.ts");
}
if (!process.env.CI) {
// make sure people building locally automatically do not track changes to version file
try {
cp.execSync("git update-index --skip-worktree src/version.ts");
} catch (error) {
console.warn("Warning: Could not update git index. This may be due to git not being installed or repository not being initialized.");
}
}


const pkg = JSON.parse(String(fs.readFileSync("./package.json", "utf8")))
const versionTsPath = "./src/version.ts";
const data = String(fs.readFileSync(versionTsPath, "utf8"));
const newData = data.replace(/"(.*)"/, `"${pkg.version}"`);
fs.writeFileSync(versionTsPath, newData);
Comment on lines +9 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add robust error handling and version validation.

The version update logic needs improvements in several areas:

  1. Error handling for file operations
  2. Version format validation
  3. Backup mechanism
  4. More precise version replacement regex
-const pkg = JSON.parse(String(fs.readFileSync("./package.json", "utf8")))
-const versionTsPath = "./src/version.ts";
-const data = String(fs.readFileSync(versionTsPath, "utf8"));
-const newData = data.replace(/"(.*)"/, `"${pkg.version}"`);
-fs.writeFileSync(versionTsPath, newData);
+try {
+  const pkg = JSON.parse(String(fs.readFileSync("./package.json", "utf8")));
+  
+  // Validate version format
+  if (!/^\d+\.\d+\.\d+(?:-[\w.-]+)?(?:\+[\w.-]+)?$/.test(pkg.version)) {
+    throw new Error(`Invalid version format: ${pkg.version}`);
+  }
+  
+  const versionTsPath = "./src/version.ts";
+  const data = String(fs.readFileSync(versionTsPath, "utf8"));
+  
+  // Create backup
+  fs.writeFileSync(`${versionTsPath}.backup`, data);
+  
+  // More precise version replacement
+  const newData = data.replace(/export const version = "(.*?)";/, `export const version = "${pkg.version}";`);
+  
+  // Verify replacement occurred
+  if (newData === data) {
+    throw new Error("Version string not found in version.ts");
+  }
+  
+  fs.writeFileSync(versionTsPath, newData);
+} catch (error) {
+  console.error("Error updating version:", error.message);
+  process.exit(1);
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const pkg = JSON.parse(String(fs.readFileSync("./package.json", "utf8")))
const versionTsPath = "./src/version.ts";
const data = String(fs.readFileSync(versionTsPath, "utf8"));
const newData = data.replace(/"(.*)"/, `"${pkg.version}"`);
fs.writeFileSync(versionTsPath, newData);
try {
const pkg = JSON.parse(String(fs.readFileSync("./package.json", "utf8")));
// Validate version format
if (!/^\d+\.\d+\.\d+(?:-[\w.-]+)?(?:\+[\w.-]+)?$/.test(pkg.version)) {
throw new Error(`Invalid version format: ${pkg.version}`);
}
const versionTsPath = "./src/version.ts";
const data = String(fs.readFileSync(versionTsPath, "utf8"));
// Create backup
fs.writeFileSync(`${versionTsPath}.backup`, data);
// More precise version replacement
const newData = data.replace(/export const version = "(.*?)";/, `export const version = "${pkg.version}";`);
// Verify replacement occurred
if (newData === data) {
throw new Error("Version string not found in version.ts");
}
fs.writeFileSync(versionTsPath, newData);
} catch (error) {
console.error("Error updating version:", error.message);
process.exit(1);
}

Comment on lines +11 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

So that we don't need to keep the src/version.ts file around, we can just generate it on the fly perhaps?

Suggested change
const data = String(fs.readFileSync(versionTsPath, "utf8"));
const newData = data.replace(/"(.*)"/, `"${pkg.version}"`);
fs.writeFileSync(versionTsPath, newData);
const newData = `export const PD_SDK_VERSION = "${pkg.version}";`
fs.writeFileSync(versionTsPath, newData);

4 changes: 2 additions & 2 deletions packages/sdk/src/browser/async.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { AsyncResponseManager } from "../shared/async";
import type { AsyncResponseManagerOpts } from "../shared/async";
import { AsyncResponseManager } from "../shared/async.js";
import type { AsyncResponseManagerOpts } from "../shared/async.js";

export type BrowserAsyncResponseManagerOpts = {
apiHost: string;
Expand Down
6 changes: 3 additions & 3 deletions packages/sdk/src/browser/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@
// operations, like connecting accounts via Pipedream Connect. See the server/
// directory for the server client.

import { BrowserAsyncResponseManager } from "./async";
import { BrowserAsyncResponseManager } from "./async.js";
import {
AccountsRequestResponse,
BaseClient,
GetAccountOpts,
type ConnectTokenResponse,
} from "../shared";
export type * from "../shared";
} from "../shared/index.js";
export type * from "../shared/index.js";

/**
* Options for creating a browser-side client. This is used to configure the
Expand Down
11 changes: 7 additions & 4 deletions packages/sdk/src/shared/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
// This code is meant to be shared between the browser and server.
import { AsyncResponseManager } from "./async";
import { AsyncResponseManager } from "./async.js";
import type {
AsyncResponse, AsyncErrorResponse,
} from "./async";
import type { V1Component } from "./component";
export * from "./component";
} from "./async.js";
import type { V1Component } from "./component.js";
export * from "./component.js";
import { version as sdkVersion } from "../version.js";

type RequestInit = globalThis.RequestInit;

Expand Down Expand Up @@ -290,6 +291,7 @@ export interface AsyncRequestOptions extends RequestOptions {
* A client for interacting with the Pipedream Connect API on the server-side.
*/
export abstract class BaseClient {
version = sdkVersion;
protected apiHost: string;
protected abstract asyncResponseManager: AsyncResponseManager;
protected readonly baseApiUrl: string;
Expand Down Expand Up @@ -351,6 +353,7 @@ export abstract class BaseClient {

const headers: Record<string, string> = {
...customHeaders,
"X-PD-SDK-Version": sdkVersion,
"X-PD-Environment": this.environment,
};

Expand Down
2 changes: 2 additions & 0 deletions packages/sdk/src/version.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// DO NOT EDIT, SET AT BUILD TIME
export const version = "0.0.0"
Loading