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.

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

Full rewrite #35

Closed
wants to merge 72 commits into from
Closed

Full rewrite #35

wants to merge 72 commits into from

Conversation

wojtekmaj
Copy link

@wojtekmaj wojtekmaj commented Aug 24, 2023

Following support case no. 5388464 I'm happy to raise this PR with a full rewrite of Jotform API Node.js client.

Fixes #15
Fixes #20
Fixes #27
Fixes #29
Fixes #33

What's been done

New features

  • Added full TypeScript support: all methods and parameters are typed and documented using official Jotform API documentation:
    image
  • Added full ESM support.
  • Added support for customHeaders, allowing use of jf-team-id header.
  • Added new methods:
    • getFormReports
    • getFormReport
    • createFormReport
    • deleteFormReport
    • deleteFormSubmission
    • deleteReport
    • getPlan

Changes

  • All methods now consistently throw on invalid data (e.g.: addFormQuestions missing formID or passed questionData that isn't an object).
  • [breaking] Replaced Q.Promise with native Promise - technically, drops support for Node.js 0.10.48 (released back in October 2016). This also slightly changes the API, because Q.Promise used .fail(), and native Promise uses .catch() to catch errors.
  • [breaking] Replaced request with Fetch API - native Fetch API was added in Node.js 16.15.0 and 17.5.0 (hidden behind a flag) and is enabled by default since Node 18.0.0. Technically, Node.js backwards compatibility can be extended by adding node-fetch polyfill if needed. I personally consider this waste of time, as Node 18 is already LTS. Fixes Vulnerable Dependencies in Package #33.

Maintenance

  • Adapted project to modern (and opinionated) development workflow standards.
    • Migrated package to TypeScript.
    • Added ESLint with eslint-config-wojtekmaj config.
    • Added Prettier for auto-formatting.
    • Added husky + pretty-quick to ensure files are auto-formatted before commit.
    • Added full E2E testing suite that talks with real Jotform API.
  • Rewritten code using ESM modules syntax - while CJS build is still provided for backwards compatibility, the source code is 100% ESM.
  • Refactored all methods, avoiding code repetition, avoiding error prone URL creation.
  • Updated documentation, most importantly documenting custom API URLs and usage with Jotform Teams. Fixes No EU Protected Mode #29.

Bug fixes

  • Fixed errors reported by ESLint.
    • Fixed non-functional addFormToFolder method.
    • Fixed fullText option ignored in getSubmissions method.
  • Fixed request hanging if response.statusCode was 200, but body.responseCode was not 200. Fixes Bad handling of API errors in library #27.
  • Fixed invalid URLs created in getForms and getSubmissions method when fullText and/or nocache params were passed.
  • Fixed incorrect handling of plaintext responses from the API (yes, there are some!).

What still needs to be done

  • In order to ensure E2E tests will continue to work:
    • Make sure to set JOTFORM_API_KEY GitHub Actions secret with a value that I'll provide to you via Jotform support channel.
    • Make sure the API key I provided does not have limits (I've used free account).
    • Consider cloning forms, submissions, folders and subfolders from my test account ([email protected]), update all TEST_*_ID in index.spec.ts file to gain full control over the data used in E2E tests and update the API key to an unlimited one.
  • Some tests are skipped because of Jotform API issues I've encountered:
    • getHistory tests are skipped because it takes super long for /user/history to respond - I'm looking for an advice to resolve this, e.g. limit the amount of data requested?
    • getSubusers tests are skipped because I'm getting "User is not Allowed" error.
    • deleteFolder tests are skipped because DELETE requests to /folder/{folderID} are not responded to - this has been already reported to you via support case no. 5393450.

@yashvesikar
Copy link

Would love to know when this might be merged upstream in a new official release?

@wojtekmaj
Copy link
Author

@yashvesikar Me too :D Feel free to test my fork in the meantime!

@yashvesikar
Copy link

@yashvesikar Me too :D Feel free to test my fork in the meantime!

Yep! I have been using it and have not run into any issues yet. Would be good to get this in the official release though.

@itsalysialynn
Copy link

Any update on when this will be merged in? We are using the existing one now and it's a bit painful!

@wojtekmaj
Copy link
Author

wojtekmaj commented Nov 25, 2023

@itsalysialynn For now, feel welcome to use @wojtekmaj/jotform. It's 1:1 as this PR proposes.

This PR gets merged? Great! Remove "@wojtekmaj/" and you're ready to go.

This PR doesn't get merged? Great! You're already using the only modern, maintained Jotform JS client.

;)

@wojtekmaj wojtekmaj closed this Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants