search

saadtazi / firefox-profile-js

Firefox Profile creation using nodejs and CLI
MIT License
60 stars 30 forks source link

fs-extra doesn't seem to be exporting an rmSync method as native nodejs fs module does #128

Closed rpl closed 3 years ago

rpl commented 3 years ago

While running web-ext unit tests locally I did notice the following error being logged when mocha did run all tests and it is exiting:

[firefox-profile] cannot delete profileDir on exit /tmp/firefox-profile2Tntmz/ TypeError: fs.rmSync is not a function
    at FirefoxProfile._cleanOnExit (/.../web-ext/node_modules/firefox-profile/lib/firefox_profile.js:288:10)
    at process.FirefoxProfile.onExit (/.../web-ext/node_modules/firefox-profile/lib/firefox_profile.js:170:12)
    at process.emit (events.js:326:22)
    at process.EventEmitter.emit (domain.js:483:12)
    at process.emit (/.../web-ext/node_modules/addons-linter/node_modules/source-map-support/source-map-support.js:516:21)
    at process.emit (/.../web-ext/node_modules/source-map-support/source-map-support.js:516:21)
    at process.topLevelDomainCallback (domain.js:138:15)
    at process.callbackTrampoline (internal/async_hooks.js:124:14)

As the stack trace shows the error seems to be related to the changes introduced in #126 and released on npm as part of firefox-profile@4.2.1.

Based on a very quick look I think that the exception is actually expected, because fs is actually set to the "fs-extra" here:

and rmSync doesn't seem to be part of the API exposed by "fs-extra" npm package (where it seems that only removeSync is actually available), unlike the native nodejs "fs" module which does actually have an rmSync method with that signature.

Looking more closely to the fs-extra removeSync function (which was also what that method used to call before #126), it seems that it should be already calling fs.rmSync internally on nodejs version where it is actually available:

And so it seems that rolling back to the previous implementation (the one already being calling fs-extra removeSync) may be actually a reasonable fix for this issue and should still remove that directory as the new version was meant to ensure.

saadtazi commented 3 years ago

Wow! Thank you so much for the detailed explanations.

You are probably using a node version < 14.4, which doesn't support fs.rmSync

note: fs-extra contains the methods of the regular fs module plus some extra functions. So the change introduced by #126 only worked for node 14.4+...

I'm finalizing a PR to fix it. please stay tuned 😄

saadtazi commented 3 years ago

I am waiting for travis-ci to grant me open-source credits: they closed travis-ci.org "recently" and the switch to travis-ci.com for open-source projects requires some communications/manual approvals with their support team...

saadtazi commented 3 years ago

I just released v4.2.2. Please let me know if it fixes your issue.

rpl commented 3 years ago

Wow! Thank you so much for the detailed explanations.

You are probably using a node version < 14.4, which doesn't support fs.rmSync

note: fs-extra contains the methods of the regular fs module plus some extra functions. So the change introduced by #126 only worked for node 14.4+...

That was it! I use mainly the last node 12 while working on web-ext (as it is the minimum nodejs version still supported by web-ext package).

I just released v4.2.2. Please let me know if it fixes your issue.

I just tested locally the related Renovate bot PR (mozilla/web-ext#2344) and I can confirm you that the issue is fixed by v4.2.2 :tada:

Thanks a lot! :heart: