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.

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

Jump to bottom

Chore/remove localhost url #228

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+29 −9
Open

Chore/remove localhost url #228

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
90702e7
chore: remove localhost value for AGENT_INTERNAL_URL
josephemswiler Nov 18, 2024
5f10d65
chore: add value to tests for env variable AGENT_INTERNAL_URL
josephemswiler Nov 26, 2024
b25ace3
chore: add test for nullish AGENT_INTERNAL_URL value
josephemswiler Nov 26, 2024
f1548d2
feat: fix tests
bangarang Nov 26, 2024
4543ff5
feat: remove the log and variable completely
bangarang Nov 26, 2024
34e9caf
chore: changeset
bangarang Nov 26, 2024
aa2e9c4
feat: move check to inside the AuthenticatecClient
bangarang Nov 26, 2024
0dd6626
feat: Add documentation for listener.use()
bangarang Aug 28, 2024
d6d43b0
feat: remove throwing errors for apiUrl
bangarang Nov 27, 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
7 changes: 5 additions & 2 deletions packages/configure/src/utils/authenticated.client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ import { Configuration, DefaultApi } from '@flatfile/api'
// TODO: We will need to make this conditional depending on if it's in the NodeVM or the Browser
import fetch, { RequestInit } from 'node-fetch'

const FLATFILE_API_URL =
process.env.AGENT_INTERNAL_URL || 'http://localhost:3000'
const FLATFILE_API_URL = process.env.AGENT_INTERNAL_URL

if (FLATFILE_API_URL == null) {
throw new Error('AGENT_INTERNAL_URL must be set in the environment')
}

export class AuthenticatedClient {
private _api?: DefaultApi
Expand Down
8 changes: 6 additions & 2 deletions packages/listener/src/events/authenticated.client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@ export class AuthenticatedClient {
public _apiUrl?: string

constructor(accessToken?: string, apiUrl?: string) {
const FLATFILE_API_URL =
CrossEnvConfig.get('AGENT_INTERNAL_URL') || 'http://localhost:3000'
const FLATFILE_API_URL = CrossEnvConfig.get('AGENT_INTERNAL_URL')
console.log('FLATFILE_API_URL', FLATFILE_API_URL)
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 console.log statement

Production code should not contain console.log statements, especially ones that might expose sensitive configuration information. Consider using a proper logging system if debugging is needed.

-    console.log('FLATFILE_API_URL', FLATFILE_API_URL)
📝 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
console.log('FLATFILE_API_URL', FLATFILE_API_URL)

if (!FLATFILE_API_URL) {
throw new Error('AGENT_INTERNAL_URL must be set in the environment')
}

const bearerToken = CrossEnvConfig.get('FLATFILE_BEARER_TOKEN')

this._accessToken = accessToken || bearerToken || '...'
Expand Down
14 changes: 14 additions & 0 deletions packages/listener/src/events/event.handler.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import { CrossEnvConfig } from '@flatfile/cross-env-config'
import { EventHandler } from './event.handler'

describe('EventHandler', () => {
let testFn: jest.Mock

if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
}

beforeEach(() => {
testFn = jest.fn()
})
Expand Down Expand Up @@ -86,4 +91,13 @@ describe('EventHandler', () => {
expect(testFn).toHaveBeenCalledTimes(1)
})
})

describe('AGENT_INTERNAL_URL', () => {
test('throws error when not set', () => {
process.env.AGENT_INTERNAL_URL = undefined
expect(() => new EventHandler()).toThrow(
'AGENT_INTERNAL_URL must be set in the environment'
)
})
})
})
14 changes: 14 additions & 0 deletions packages/listener/src/flatfile.listener.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import { CrossEnvConfig } from '@flatfile/cross-env-config'
import { FlatfileListener } from './flatfile.listener'

describe('Client', () => {
let testFn: jest.Mock

if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
}
Comment on lines +7 to +9
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

Move environment setup into appropriate test lifecycle hooks

The environment variable setup should be moved into appropriate test lifecycle hooks (like beforeAll/afterAll) to ensure proper test isolation and cleanup.

Consider this approach:

- if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
-   process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
- }

+ let originalAgentInternalUrl: string | undefined;
+ 
+ beforeAll(() => {
+   originalAgentInternalUrl = process.env.AGENT_INTERNAL_URL;
+   if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
+     process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
+   }
+ });
+ 
+ afterAll(() => {
+   if (originalAgentInternalUrl === undefined) {
+     delete process.env.AGENT_INTERNAL_URL;
+   } else {
+     process.env.AGENT_INTERNAL_URL = originalAgentInternalUrl;
+   }
+ });
📝 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 (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
}
let originalAgentInternalUrl: string | undefined;
beforeAll(() => {
originalAgentInternalUrl = process.env.AGENT_INTERNAL_URL;
if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
}
});
afterAll(() => {
if (originalAgentInternalUrl === undefined) {
delete process.env.AGENT_INTERNAL_URL;
} else {
process.env.AGENT_INTERNAL_URL = originalAgentInternalUrl;
}
});


beforeEach(() => {
testFn = jest.fn()
})
Expand Down Expand Up @@ -150,4 +155,13 @@ describe('Client', () => {
expect(testFn).toHaveBeenCalledTimes(3)
})
})

describe('AGENT_INTERNAL_URL', () => {
test('throws error when not set', () => {
delete process.env.AGENT_INTERNAL_URL
expect(() => new FlatfileListener()).toThrow(
'AGENT_INTERNAL_URL must be set in the environment'
)
})
})
})
Loading