Skip to content

Require project environment in SDK #14679

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
bzwrk merged 22 commits into from
Nov 20, 2024
Merged

Require project environment in SDK #14679

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
186822e
Require project environment in SDK
bzwrk Nov 18, 2024
d34e259
Merge branch 'master' into biz/dj-2678-make-environment-mandatory-in-…
bzwrk Nov 18, 2024
647e18c
Refactor environment check into its own method
bzwrk Nov 18, 2024
d91741b
Define a new type for project environment
bzwrk Nov 19, 2024
c1781ed
Fix comment and update CHANGELOG.md
bzwrk Nov 19, 2024
40bab31
Fix linter errors
bzwrk Nov 19, 2024
7753a5b
Fix linter errors
bzwrk Nov 19, 2024
a904e53
Update package lock
bzwrk Nov 19, 2024
5efcc32
Update gitignore
bzwrk Nov 19, 2024
45b4875
Add eslint config
bzwrk Nov 19, 2024
7abedce
Apply eslint
bzwrk Nov 19, 2024
8de3f44
Apply linter suggestions
bzwrk Nov 19, 2024
16704ee
Merge branch 'master' into biz/dj-2678-make-environment-mandatory-in-…
bzwrk Nov 19, 2024
9d5e709
Fix markdown syntax errors
bzwrk Nov 19, 2024
d0362d0
Fix tests
bzwrk Nov 19, 2024
2070190
Merge branch 'master' into biz/dj-2678-make-environment-mandatory-in-…
bzwrk Nov 19, 2024
87fb597
Update pnpm lock
bzwrk Nov 19, 2024
5b86f27
Remove pnpm lock changes
bzwrk Nov 19, 2024
041a594
Merge branch 'master' into biz/dj-2678-make-environment-mandatory-in-…
bzwrk Nov 20, 2024
a171866
Remove eslint configs
bzwrk Nov 20, 2024
96fc56b
Undo eslint change
bzwrk Nov 20, 2024
044daed
Revert reversion of eslint changes
bzwrk Nov 20, 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/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
<!-- markdownlint-disable MD024 -->
# Changelog

## [1.0.5] - 2024-11-18

### Changed

- The backend client used to default to `production` if the environment was

Check failure on line 8 in packages/sdk/CHANGELOG.md

View workflow job for this annotation

GitHub Actions / test

Trailing spaces 

packages/sdk/CHANGELOG.md:8:76 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md009.md

Check failure on line 8 in packages/sdk/CHANGELOG.md

View workflow job for this annotation

GitHub Actions / test

Trailing spaces 

packages/sdk/CHANGELOG.md:8:76 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md009.md
not specified. Now `environment` is a required argument for

Check failure on line 9 in packages/sdk/CHANGELOG.md

View workflow job for this annotation

GitHub Actions / test

Trailing spaces 

packages/sdk/CHANGELOG.md:9:62 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md009.md

Check failure on line 9 in packages/sdk/CHANGELOG.md

View workflow job for this annotation

GitHub Actions / test

Trailing spaces 

packages/sdk/CHANGELOG.md:9:62 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md009.md
`createBackendClient` and must be one of `production` or `development`.

Check failure on line 10 in packages/sdk/CHANGELOG.md

View workflow job for this annotation

GitHub Actions / test

Trailing spaces 

packages/sdk/CHANGELOG.md:10:74 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md009.md

Check failure on line 10 in packages/sdk/CHANGELOG.md

View workflow job for this annotation

GitHub Actions / test

Trailing spaces 

packages/sdk/CHANGELOG.md:10:74 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md009.md

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 19, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Based on the code analysis, I can now generate the final response:

Breaking change confirmed - version bump needed

The change is indeed a breaking change as evidenced by:

  • Type definition shows environment was optional: environment?: ProjectEnvironment
  • New code enforces it as required through ensureValidEnvironment method
  • Error is thrown if environment is not provided: "Project environment is required"
  • Previous default behavior of using "production" is removed

The version should be bumped to either:

  • Major version (2.0.0) - recommended for breaking changes
  • Minor version (1.1.0) - minimum for backward compatibility breaks
🔗 Analysis chain

Version bump should follow semver for breaking changes

The change makes environment a required parameter, which is a breaking change as it modifies the existing API contract. According to semantic versioning:

  • Breaking changes should increment the major version (2.0.0)
  • Or at minimum, the minor version (1.1.0)

Currently, only the patch version is incremented (1.0.4 -> 1.0.5).

Let's verify if this is indeed a breaking change by checking the usage:

Let me gather more context about the breaking change by checking the implementation and previous behavior.

Let me check the type definition of BackendClientOpts to verify if environment is indeed a required parameter.

Let me try a different pattern to find the type definitions and environment-related code.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for createBackendClient usage without environment parameter
ast-grep --pattern 'createBackendClient($$$)'

Length of output: 155

Script:

#!/bin/bash
# Check the implementation of createBackendClient
ast-grep --pattern 'function createBackendClient($_) {
  $$$
}'

# Check the test file content to understand the usage
cat packages/sdk/src/server/__tests__/server.test.ts

Length of output: 24492

Script:

#!/bin/bash
# Check the type definition of BackendClientOpts
ast-grep --pattern 'interface BackendClientOpts {
  $$$
}'

# Also check for any environment-related type definitions
ast-grep --pattern 'type $_Environment'

Length of output: 102

Script:

#!/bin/bash
# Search for any type definitions in the SDK
rg "type|interface" packages/sdk/src/ -A 5

# Search for environment-related code
rg "environment" packages/sdk/src/ -B 2 -A 2

Length of output: 24981

🧰 Tools
🪛 GitHub Check: test

[failure] 8-8: Trailing spaces
packages/sdk/CHANGELOG.md:8:76 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md009.md

[failure] 9-9: Trailing spaces
packages/sdk/CHANGELOG.md:9:62 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md009.md

[failure] 10-10: Trailing spaces
packages/sdk/CHANGELOG.md:10:74 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md009.md

## [1.0.4] - 2024-11-15

### Changed
Expand Down
2 changes: 1 addition & 1 deletion packages/sdk/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@pipedream/sdk",
"version": "1.0.4",
"version": "1.0.5",
"description": "Pipedream SDK",
"main": "dist/server/index.js",
"module": "dist/server/index.js",
Expand Down
16 changes: 14 additions & 2 deletions packages/sdk/src/server/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@
clientSecret: string;
};

/**
* The environment in which the server client is running.
*/
export type ProjectEnvironment = "development" | "production"

Check failure on line 22 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

Missing semicolon

/**
* Options for creating a server-side client.
* This is used to configure the BackendClient instance.
Expand All @@ -25,7 +30,7 @@
* The environment in which the server client is running (e.g., "production",
* "development").
*/
environment?: string;
environment?: ProjectEnvironment;

/**
* The credentials to use for authentication against the Pipedream API.
Expand Down Expand Up @@ -353,7 +358,8 @@
* @param opts - The options for configuring the server client.
*/
constructor(opts: BackendClientOpts) {
this.environment = opts.environment ?? "production";
this.ensureValidEnvironment(opts.environment)

Check failure on line 361 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

Missing semicolon
this.environment = opts.environment!!

Check failure on line 362 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

Forbidden extra non-null assertion

Check failure on line 362 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

Missing semicolon
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 19, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove redundant non-null assertion

The double non-null assertion (!!) is unnecessary since ensureValidEnvironment already validates the environment parameter.

-    this.ensureValidEnvironment(opts.environment)
-    this.environment = opts.environment!!
+    this.ensureValidEnvironment(opts.environment);
+    this.environment = opts.environment;
📝 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
this.ensureValidEnvironment(opts.environment)
this.environment = opts.environment!!
this.ensureValidEnvironment(opts.environment);
this.environment = opts.environment;
🧰 Tools
🪛 GitHub Check: Lint Code Base

[failure] 361-361:
Missing semicolon

[failure] 362-362:
Forbidden extra non-null assertion

[failure] 362-362:
Missing semicolon

🪛 Biome

[error] 362-362: Forbidden extra non-null assertion.

Safe fix: Remove extra non-null assertion.

(lint/suspicious/noExtraNonNullAssertion)

🪛 eslint

[error] 361-362: Missing semicolon.

(@typescript-eslint/semi)

[error] 362-362: Forbidden extra non-null assertion.

(@typescript-eslint/no-extra-non-null-assertion)


this.projectId = opts.projectId;
if (!this.projectId) {
Expand All @@ -370,6 +376,12 @@
this.oauthClient = this.newOauthClient(opts.credentials, this.baseApiUrl);
}

private ensureValidEnvironment(environment?: string) {
if (!environment || !["development", "production"].includes(environment)) {

Check failure on line 380 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

A linebreak is required after '['

Check failure on line 380 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

There should be a linebreak after this element

Check failure on line 380 in packages/sdk/src/server/index.ts

View workflow job for this annotation

GitHub Actions / Lint Code Base 

A linebreak is required before ']'
throw new Error("Project environment is required. Supported environments are development and production.");
}
}

private newOauthClient(
{
clientId,
Expand Down
Loading