-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
Changes from all commits
3f02372
039eb96
5cc123b
dfad322
29ec2ae
2663c9a
facbdcd
4a42019
c204d9d
24473f9
e8e44f0
ea290cb
801c5aa
43bd76b
19bf059
cdf99e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,8 @@ | ||||||||||||||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||||||||||||||
make -j2 server delayed-open | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
server: | ||||||||||||||||||||||||||||||||||||||
cd ../.. && python -m http.server | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+4
to
+5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enhance server security and configuration The current server implementation has several issues:
+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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve cross-platform compatibility and error handling The current implementation has platform-specific issues:
+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
Suggested change
|
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> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
default: | ||
@node index.mjs |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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();
|
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+9
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add robust error handling and version validation. The version update logic needs improvements in several areas:
-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
Suggested change
Comment on lines
+11
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So that we don't need to keep the
Suggested change
|
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" |
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.
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.