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

perf(js-sdk): remove axios #615

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

MonsterDeveloper
Copy link
Contributor

@MonsterDeveloper MonsterDeveloper commented Sep 3, 2024

Changes made

  • used fetch instead of axios
  • added FirecrawlApiError class
  • added polyfill from cross-fetch (only in environments where fetch is not available)
  • added jsdoc for mapUrl method
  • support for passing a JsonSchema directly into the schema option

TODO

  • rewrite tests
  • resolve todos in code
  • update docs

I would recommend merging #611 first, then #614 and finally this PR.

@nickscamara
Copy link
Member

This is awesome! @MonsterDeveloper Thank you! Excited for it.

@MonsterDeveloper
Copy link
Contributor Author

@nickscamara @mogery hi! Any updates on the related PR's #611 and #614? Looking forward to get going on this one.

@mogery
Copy link
Member

mogery commented Sep 11, 2024

Sorry, slipped my mind. Taking a look today.

@mogery
Copy link
Member

mogery commented Sep 12, 2024

Merged both related PRs! Excited about this one :)

@MonsterDeveloper
Copy link
Contributor Author

MonsterDeveloper commented Sep 13, 2024

@mogery hi again! I ended up cooking up something big, starting with the CrawlWatcher:

  • Added ESLint with basic rules and Prettier with the default config
  • Restructured the code into multiple files instead of one big index.ts
  • Started some tests with Vitest (npm run test to run them)

Points up for debate:

  • The ESLint and Prettier config we want to use
  • File naming conventions (I used camelCase)
  • Whether to use fixtures in the current CrawlWatcher tests and the upcoming FirecrawlApp tests (I haven't added any yet)

So far, I've completed:

  • getFetch and its tests
  • isZodSchema and its tests
  • CrawlWatcher and its tests (just minor adjustments, API remains the same)

Could you please review these files and their respective tests? The FirecrawlApp is still in progress.

@mogery mogery marked this pull request as ready for review September 13, 2024 11:50
@mogery mogery marked this pull request as draft September 13, 2024 11:51
@mogery
Copy link
Member

mogery commented Sep 13, 2024

Looking great! (clicked the wrong button when in un-drafted there haha)

Will talk with Nick today on whether we need to retain v0 tests for FirecrawlApp or if only v1 tests need to be ported.

Prettier and ESLint is a good call. Thanks!

@MonsterDeveloper
Copy link
Contributor Author

Will talk with Nick today on whether we need to retain v0 tests for FirecrawlApp or if only v1 tests need to be ported.

Please do:) Let me know how it goes.

I'm not quite sure what is the status of v0 for now (deprecated or still receiving patches), but if it's no longer maintained you could do a major version bump for js-sdk and I'll only focus on v1 methods and tests.

@mogery
Copy link
Member

mogery commented Sep 13, 2024

I'm not quite sure what is the status of v0 for now (deprecated or still receiving patches), but if it's no longer maintained you could do a major version bump for js-sdk and I'll only focus on v1 methods and tests.

v0 is still actively receiving patches, we will not deprecate it yet. We've already done a major bump on js-sdk to 1.x.x for v1.

@MonsterDeveloper
Copy link
Contributor Author

We've already done a major bump on js-sdk to 1.x.x for v1.

Got it. Does this mean that 0.x version of the SDK is for v0 API and 1.x is for v1? If so, then it seems like there should be 2 folders in the repo: one for SDK v0, and another for the current for v1, since they’re both maintained and you probably want to have an option to publish updates both for the current and old version of the SDK.

@mogery
Copy link
Member

mogery commented Sep 18, 2024

We've already done a major bump on js-sdk to 1.x.x for v1.

Got it. Does this mean that 0.x version of the SDK is for v0 API and 1.x is for v1? If so, then it seems like there should be 2 folders in the repo: one for SDK v0, and another for the current for v1, since they’re both maintained and you probably want to have an option to publish updates both for the current and old version of the SDK.

v0 is meant to get server-side fixes only.

@MonsterDeveloper
Copy link
Contributor Author

@mogery hi! Sorry for a delayed response, was doing some travels.

So since both v0 and v1 support is needed, the optimal solution would be to have 2 separate classes for the different versions of the API. Mixing these 2 together in one class would require weird prefixes which don't contribute to readability and structure, to say the least.

I've just looked through the API reference and I can't seem to find the WebSocket docs for v0. It was only introduced in v1, am I right?

@mogery
Copy link
Member

mogery commented Sep 30, 2024

Hey! Sorry for the delayed response haha. Yes, that's in V1 only.

@twlite
Copy link
Contributor

twlite commented Oct 29, 2024

uhdici.request is faster than fetch, would it make sense to use undici?

@MonsterDeveloper
Copy link
Contributor Author

uhdici.request is faster than fetch, would it make sense to use undici?

Haven't heard about it being faster than fetch. I guess that depends on the runtime you're running it in.

But anyway, the goal here is to eliminate dependencies, as the millisecond performance of making an HTTP request is not a priority in this lib I assume.

@twlite
Copy link
Contributor

twlite commented Oct 30, 2024

uhdici.request is faster than fetch, would it make sense to use undici?

Haven't heard about it being faster than fetch. I guess that depends on the runtime you're running it in.

But anyway, the goal here is to eliminate dependencies, as the millisecond performance of making an HTTP request is not a priority in this lib I assume.

It is indeed slower than undici.request as mentioned in nodejs/undici#1203 (comment)

Plus, Node's global fetch is actually coming from undici.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants