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

Migrate rimraf to node:fs in firebase-tools #126

Open
echedaleux opened this issue Oct 11, 2024 · 10 comments
Open

Migrate rimraf to node:fs in firebase-tools #126

echedaleux opened this issue Oct 11, 2024 · 10 comments

Comments

@echedaleux
Copy link

echedaleux commented Oct 11, 2024

Use built-in Node APIs instead rimraf.

@echedaleux echedaleux changed the title Migrate rimraf to node:fs in firebase-tools Migrate rimraf to node:fs in firebase-tools Oct 11, 2024
@echedaleux echedaleux changed the title Migrate rimraf to node:fs in firebase-tools Migrate rimraf to node:fs in firebase-tools Oct 11, 2024
@echedaleux
Copy link
Author

PR opened here on the firebase-tools repository : firebase/firebase-tools#7826

@Namchee
Copy link

Namchee commented Oct 15, 2024

This repository also use the following dependencies:

  • glob
  • strip-ansi
  • chokidar v3
  • node-fetch (do we still need this?)

@43081j
Copy link
Collaborator

43081j commented Oct 16, 2024

given they specify:

  "engines": {
    "node": ">=18.0.0 || >=20.0.0"
  },

A few others I noticed:

  • Remove abort-controller and use the built in AbortController (available in node >=15)
  • lodash (but not all have native equivalents so we'd need to introduce new dependencies, like dlv for getting deep props)
  • ora with nanospinner (ping one of us in discord if you do this maybe, since there's a chance we use one from tinylibs instead - TBD)

I suspect if you look at the graph or the pkg-size result, you'll see yaml is pretty chunky too. but I'm not sure if there are alternatives (that may just be how big a yaml parser has to be)

@jsaguet
Copy link

jsaguet commented Oct 22, 2024

PR to remove strip-ansi firebase/firebase-tools#7860

@jsaguet
Copy link

jsaguet commented Nov 2, 2024

node-fetch, abort-controller, form-data and proxy-agent dependencies are all linked together.

I've worked on a PR to replace everything by native fetch.

It seems to be working but every test relies on nock which does not support native fetch on the current version (13).
Updating everything to undici's MockAgent is a huge work and I'm not sure it is really worth it considering fetch support is planned for the next release (14) of nock.

Are there other examples of migrations from node-fetch to fetch so that I may find another way ?

@43081j
Copy link
Collaborator

43081j commented Nov 2, 2024

Do you have a link to the branch? And an example test that fails

Maybe we can figure something out. Would be good to see some code though so I can better follow what you mean

@jsaguet
Copy link

jsaguet commented Nov 3, 2024

Sure, here is the branch: https://github.com/jsaguet/firebase-tools/tree/feat/remove-node-fetch

My point was that every test in firebase-tools that mocks http requests does it with the nock package.
nock works well with node-fetch because both are based on node:http / node:https modules.

However, nock does not work with fetch yet so moving to native fetch breaks all of those tests because network calls are not mocked anymore.

Migrating those tests to another tool would be a huge effort with very low value and the PR might by rejected by the maintainers because they are used to nock.

We could just wait for the next release of nock which will support mocking fetch calls.

I was asking if there was known drop-in replacement to nock that can mock fetch calls because I could easily switch to such tool right now instead of waiting for the next release of nock.

@43081j
Copy link
Collaborator

43081j commented Nov 4, 2024

That makes sense. Really it would be nice to just stub fetch out like any other function. But it would be a big refactor, you're right

I'm not aware of a nock alternative either unfortunately. Do you know if there's a tracking issue for them supporting fetch? So we can follow

@jsaguet
Copy link

jsaguet commented Nov 4, 2024

I found this one nock/nock#2397

@43081j
Copy link
Collaborator

43081j commented Nov 4, 2024

aha yes it looks like nock@beta might do what you need!

they already shipped experimental support it seems

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

No branches or pull requests

4 participants