refactor: @formbricks/api package #782

ShubhamPalriwala · 2023-09-06T07:52:21Z

What does this PR do?

This is currently a Draft PR for feedback on the rewritten package! Once the changes are gtg, it will be renamed from the current formbricks-api-wrapper to our formbricks/api and the old package will be deleted! This is not done till now so that its easier to review and compare the packages!

Methods supported:

- api.client.response.create()
- api.client.response.update()
- api.client.display.markSurveyAsRespondedForPerson()
- api.client.display.markSurveyAsDisplayedForPerson()

ToDo before Merging

Remove old package
Rename this as per the old package
Update README when migrating to the directory
Find usages of the current API package in the monorepo and update them accordingly

Type of change

Chore (refactoring code, technical debt, workflow improvements)

How should this be tested?

Create a new file and call the package methods after initialising it!
Now access the available methods

Checklist

vercel · 2023-09-06T07:52:27Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
formbricks-cloud	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 1, 2023 9:50am

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
formbricks-com	⬜️ Ignored (Inspect)	Visit Preview		Oct 1, 2023 9:50am

github-actions · 2023-09-06T07:52:36Z

Thank you for following the naming conventions for pull request titles! 🙏

github-actions · 2023-09-06T08:03:41Z

📦 Next.js Bundle Analysis for @formbricks/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

github-actions · 2023-09-06T08:28:56Z

📦 Next.js Bundle Analysis for @formbricks/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

mattinannt

@ShubhamPalriwala looks great! thank you for this first draft :-)

mattinannt · 2023-09-06T08:31:00Z

packages/formbricks-api-wrapper/src/utils/makeRequest.ts

@@ -0,0 +1,37 @@
+import fetch from "node-fetch";


In our repository the package is mainly used client side (formbricks-js) but should also work server side in. What do we need to do to make it work both client and server side?

I added the node-fetch for that purpose itself! For non-browser interactions, to have the fetch, we can use the node-fetch packale

mattinannt · 2023-09-06T08:32:42Z

packages/formbricks-api-wrapper/src/utils/makeRequest.ts

+import { Result, err, ok, wrapThrows } from "@formbricks/errors";
+import { NetworkError, ApiResponse } from "../types/index";
+
+export async function makeRequest<T = any, E = any>(


Please take a look at the errors package and try to simplify this generic type as this is hard to understand. (If it makes sense?)

done! please take a look now

mattinannt · 2023-09-06T08:35:05Z

packages/formbricks-api-wrapper/src/api/client/index.ts

+import { ApiConfig } from "../../types";
+
+export class Client {
+  response: ResponseAPI;


I would prefer plural responses as it is also used that way in our api route. What do you think?

I think, having singular is better since it's a method name and can differ from the API route! Singular keeps it simple and does not set any expectations

github-actions · 2023-09-06T12:30:39Z

📦 Next.js Bundle Analysis for @formbricks/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

github-actions · 2023-09-07T10:18:44Z

📦 Next.js Bundle Analysis for @formbricks/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

mattinannt · 2023-09-08T05:57:54Z

@ShubhamPalriwala we are also using the api package in the onboarding in apps/web/app/(app)/onboarding/page.tsx (passed through the js package). Can you please adjust this also to the new syntax? 🤗

mattinannt · 2023-09-13T06:59:02Z

@ShubhamPalriwala Can you please solve the merge conflicts and also update the Formbricks Onboarding as mentioned in the comment above? 😊🙏

mattinannt · 2023-09-13T06:59:50Z

packages/api/src/api/client/display.ts

+    this.apiHost = baseUrl;
+  }
+
+  async markSurveyAsDisplayedForPerson({


maybe we should just name it markResponded ?

done, new survey methods are:
markDisplayedForPerson, markResponded

mattinannt · 2023-09-13T07:00:33Z

packages/api/src/types/index.ts

@@ -0,0 +1,15 @@
+export type NetworkError = {


can we maybe move this error to @formbricks/types/errors?

done, thanks for pointing this out

…shubham/for-1151-rewrite-formbricks-api-package

ShubhamPalriwala · 2023-09-14T01:43:12Z

hey, i've updated the PR as per your comments and resolved merge conflicts as well! however, i'm running into the following build issues:

@formbricks/demo:build: - info Linting and checking validity of types...
@formbricks/formbricks-com:build: - info Creating an optimized production build...
@formbricks/demo:build:
@formbricks/demo:build: ./pages/_app.tsx
@formbricks/demo:build: 34:6  Warning: React Hook useEffect has a missing dependency: 'router.events'. Either include it or remove the dependency array.  react-hooks/exhaustive-deps
@formbricks/demo:build:
@formbricks/demo:build: info  - Need to disable some ESLint rules? Learn more here: https://nextjs.org/docs/basic-features/eslint#disabling-rules
@formbricks/demo:build: - info Creating an optimized production build...
@formbricks/demo:build: Failed to compile.
@formbricks/demo:build:
@formbricks/demo:build: ../../packages/js/dist/chunk-HIQASOGE.mjs
@formbricks/demo:build: Module not found: Can't resolve 'fs'
@formbricks/demo:build:
@formbricks/demo:build: https://nextjs.org/docs/messages/module-not-found
@formbricks/demo:build:
@formbricks/demo:build: Import trace for requested module:
@formbricks/demo:build: ../../packages/js/dist/index.mjs
@formbricks/demo:build: ./pages/_app.tsx
@formbricks/demo:build:
@formbricks/demo:build: ../../packages/js/dist/index.mjs
@formbricks/demo:build: Module not found: Can't resolve 'net'
@formbricks/demo:build:
@formbricks/demo:build: https://nextjs.org/docs/messages/module-not-found
@formbricks/demo:build:
@formbricks/demo:build: Import trace for requested module:
@formbricks/demo:build: ./pages/_app.tsx
@formbricks/demo:build:
@formbricks/demo:build:
@formbricks/demo:build: > Build failed because of webpack errors
@formbricks/demo:build:  ELIFECYCLE  Command failed with exit code 1.
@formbricks/demo:build: ERROR: command finished with error: command (/home/shubham/Desktop/formbricks/apps/demo) pnpm run build exited (1)
command (/home/shubham/Desktop/formbricks/apps/demo) pnpm run build exited (1)

 Tasks:    5 successful, 8 total
Cached:    4 cached, 8 total
  Time:    33.39s
Failed:    @formbricks/demo#build

 ERROR  run failed: command  exited (1)
 ELIFECYCLE  Command failed with exit code 1.

I tried adding the "fs" module as well but then after couldn't figure out why the build was having the "net" package.
Can you help me with this? :/

mattinannt · 2023-09-18T04:24:55Z

hey, i've updated the PR as per your comments and resolved merge conflicts as well! however, i'm running into the following build issues:

@formbricks/demo:build: - info Linting and checking validity of types...
@formbricks/formbricks-com:build: - info Creating an optimized production build...
@formbricks/demo:build:
@formbricks/demo:build: ./pages/_app.tsx
@formbricks/demo:build: 34:6  Warning: React Hook useEffect has a missing dependency: 'router.events'. Either include it or remove the dependency array.  react-hooks/exhaustive-deps
@formbricks/demo:build:
@formbricks/demo:build: info  - Need to disable some ESLint rules? Learn more here: https://nextjs.org/docs/basic-features/eslint#disabling-rules
@formbricks/demo:build: - info Creating an optimized production build...
@formbricks/demo:build: Failed to compile.
@formbricks/demo:build:
@formbricks/demo:build: ../../packages/js/dist/chunk-HIQASOGE.mjs
@formbricks/demo:build: Module not found: Can't resolve 'fs'
@formbricks/demo:build:
@formbricks/demo:build: https://nextjs.org/docs/messages/module-not-found
@formbricks/demo:build:
@formbricks/demo:build: Import trace for requested module:
@formbricks/demo:build: ../../packages/js/dist/index.mjs
@formbricks/demo:build: ./pages/_app.tsx
@formbricks/demo:build:
@formbricks/demo:build: ../../packages/js/dist/index.mjs
@formbricks/demo:build: Module not found: Can't resolve 'net'
@formbricks/demo:build:
@formbricks/demo:build: https://nextjs.org/docs/messages/module-not-found
@formbricks/demo:build:
@formbricks/demo:build: Import trace for requested module:
@formbricks/demo:build: ./pages/_app.tsx
@formbricks/demo:build:
@formbricks/demo:build:
@formbricks/demo:build: > Build failed because of webpack errors
@formbricks/demo:build:  ELIFECYCLE  Command failed with exit code 1.
@formbricks/demo:build: ERROR: command finished with error: command (/home/shubham/Desktop/formbricks/apps/demo) pnpm run build exited (1)
command (/home/shubham/Desktop/formbricks/apps/demo) pnpm run build exited (1)

 Tasks:    5 successful, 8 total
Cached:    4 cached, 8 total
  Time:    33.39s
Failed:    @formbricks/demo#build

 ERROR  run failed: command  exited (1)
 ELIFECYCLE  Command failed with exit code 1.

I tried adding the "fs" module as well but then after couldn't figure out why the build was having the "net" package. Can you help me with this? :/

It seems to we an issue with node-fetch but I don't know why. But I think as we currently only use the api-package client side (inside js package and inside formbricks client components using the js package), we can just leave it out. Maybe if we need server-side support again later we can find a solution then.
I pushed a fix removing the node-fetch package.
Except for this, is this ready from your side?

ShubhamPalriwala · 2023-09-18T04:26:22Z

yessir, it's good to go 🚀

…shubham/for-1151-rewrite-formbricks-api-package

…ps://github.com/formbricks/formbricks into shubham/for-1151-rewrite-formbricks-api-package

mattinannt

@ShubhamPalriwala Looks great, thank you :-)

* init: rewritten formbricks api package * restrucure: client prepended formbricks api package * feat: cleanup error templating and node-fetch for non browser access * feat: replace package (build error for js packge due to api) * fix: rename methods & move error type * fix: imports of api via js * remove node-fetch --------- Co-authored-by: Matthias Nannt <mail@matthiasnannt.com>

init: rewritten formbricks api package

1c61b93

restrucure: client prepended formbricks api package

263a595

vercel bot deployed to Preview – formbricks-cloud September 6, 2023 08:22 View deployment

mattinannt reviewed Sep 6, 2023

View reviewed changes

feat: cleanup error templating and node-fetch for non browser access

a634ef0

vercel bot deployed to Preview – formbricks-cloud September 6, 2023 12:23 View deployment

feat: replace package (build error for js packge due to api)

4397727

vercel bot had a problem deploying to Preview – formbricks-cloud September 7, 2023 10:09 Failure

ShubhamPalriwala marked this pull request as ready for review September 7, 2023 12:12

mattinannt reviewed Sep 13, 2023

View reviewed changes

Merge branch 'main' of https://github.com/formbricks/formbricks into …

6a08693

…shubham/for-1151-rewrite-formbricks-api-package

vercel bot had a problem deploying to Preview – formbricks-cloud September 14, 2023 01:12 Failure

fix: rename methods & move error type

587fbba

vercel bot had a problem deploying to Preview – formbricks-cloud September 14, 2023 01:17 Failure

fix: imports of api via js

4c59b6f

vercel bot had a problem deploying to Preview – formbricks-cloud September 14, 2023 01:35 Failure

mattinannt self-assigned this Sep 18, 2023

remove node-fetch

6ffb34a

vercel bot deployed to Preview – formbricks-cloud September 18, 2023 04:29 View deployment

ShubhamPalriwala added 2 commits September 18, 2023 10:40

Merge branch 'main' of https://github.com/formbricks/formbricks into …

f14463d

…shubham/for-1151-rewrite-formbricks-api-package

Merge branch 'shubham/for-1151-rewrite-formbricks-api-package' of htt…

9ab5af0

…ps://github.com/formbricks/formbricks into shubham/for-1151-rewrite-formbricks-api-package

vercel bot deployed to Preview – formbricks-cloud September 18, 2023 05:17 View deployment

merge lastest changes, solve merge conflicts

68a46a5

vercel bot deployed to Preview – formbricks-cloud October 1, 2023 09:50 View deployment

mattinannt approved these changes Oct 1, 2023

View reviewed changes

mattinannt merged commit 769ceb1 into main Oct 1, 2023
11 of 12 checks passed

mattinannt deleted the shubham/for-1151-rewrite-formbricks-api-package branch October 1, 2023 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: @formbricks/api package #782

refactor: @formbricks/api package #782

ShubhamPalriwala commented Sep 6, 2023 •

edited

Loading

vercel bot commented Sep 6, 2023 •

edited

Loading

github-actions bot commented Sep 6, 2023 •

edited

Loading

github-actions bot commented Sep 6, 2023

github-actions bot commented Sep 6, 2023

mattinannt left a comment

mattinannt Sep 6, 2023

ShubhamPalriwala Sep 6, 2023

mattinannt Sep 6, 2023

ShubhamPalriwala Sep 6, 2023

mattinannt Sep 6, 2023

ShubhamPalriwala Sep 6, 2023

github-actions bot commented Sep 6, 2023

github-actions bot commented Sep 7, 2023

mattinannt commented Sep 8, 2023

mattinannt commented Sep 13, 2023

mattinannt Sep 13, 2023

ShubhamPalriwala Sep 14, 2023

mattinannt Sep 13, 2023

ShubhamPalriwala Sep 14, 2023

ShubhamPalriwala commented Sep 14, 2023

mattinannt commented Sep 18, 2023

ShubhamPalriwala commented Sep 18, 2023

mattinannt left a comment

refactor: @formbricks/api package #782

refactor: @formbricks/api package #782

Conversation

ShubhamPalriwala commented Sep 6, 2023 • edited Loading

What does this PR do?

ToDo before Merging

Type of change

How should this be tested?

Checklist

vercel bot commented Sep 6, 2023 • edited Loading

github-actions bot commented Sep 6, 2023 • edited Loading

github-actions bot commented Sep 6, 2023

📦 Next.js Bundle Analysis for @formbricks/web

github-actions bot commented Sep 6, 2023

📦 Next.js Bundle Analysis for @formbricks/web

mattinannt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 6, 2023

📦 Next.js Bundle Analysis for @formbricks/web

github-actions bot commented Sep 7, 2023

📦 Next.js Bundle Analysis for @formbricks/web

mattinannt commented Sep 8, 2023

mattinannt commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShubhamPalriwala commented Sep 14, 2023

mattinannt commented Sep 18, 2023

ShubhamPalriwala commented Sep 18, 2023

mattinannt left a comment

Choose a reason for hiding this comment

ShubhamPalriwala commented Sep 6, 2023 •

edited

Loading

vercel bot commented Sep 6, 2023 •

edited

Loading

github-actions bot commented Sep 6, 2023 •

edited

Loading