Add Node.js 12 support to react-fs #20541
Conversation
With a slight modification to an import statement in react-fs it should now support Node.js 12.
|
Hi @MylesBorins! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
I've now signed the CLA |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f49efc8:
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
I got the |
|
ok... I managed to get flow working (I think). I've never used flow before... so I had to guess at what was going on... please feel free to let me know if I did something wrong or if there is a better way to do this. originally there was an additional flow definition added for To get the flow definitions working I had to manually recreate the fs flow definitions (using the existing definitions in the repo as well as the included flow definitions for the interfaces of fs being used). My gut is that the way I did it is more verbose than needed... but at least the flow definitions work 🎉 |
|
Thanks for the PR! Do you know if the demo fully runs on Node 12 after this? I'm not sure how useful this is in practice since these packages won't be production-ready for a while, and 14 is LTS anyway. |
|
hey @gaearon I beleive that the demo does fully run on Node 12 after this change (and the one I sent to the demo)... although to be honest I didn't test it out extensively. |
scripts/flow/environment.js
Outdated
| ): Promise<mixed>; | ||
| stat(path: string, options?: ?{bigint?: boolean}): Promise<mixed>; | ||
| } | ||
| declare var promises: FSPromise; |
There was a problem hiding this comment.
I think you can put the type here inline without a class?
declare var promises: {
// ...
}
There was a problem hiding this comment.
This pattern was copied directly from the default type definitions
There was a problem hiding this comment.
scripts/flow/environment.js
Outdated
| declare var readdir: ( | ||
| path: string, | ||
| declare module 'fs' { | ||
| declare function existsSync(path: string): boolean; |
There was a problem hiding this comment.
Kind of unfortunate that we're not relying on built-in definitions here. Maybe we can just get rid of this when we update Flow and get Promises from upstream.
There was a problem hiding this comment.
So there are built in definitions that could be used but there is no promises definition for fs.promises.lstat, fs.promises.stat does not define an option, and the default fs.promises.realpath is ambiguous.
If there is an easy way to overload specific entries with flow? I couldn't figure out how to do that, but if we could overload just those three functions we could avoid the rest of the noise
|
@gaearon there are only 5 calls to fs promise APIs that were causing issues with the built in flow definitions. I've pushed f49efc8 which ignores types for those 5 calls so we can remove all the custom type information related to fs / promises and rely entirely on the built-in types elsewhere. Unsure if this approach is preferable, feel free to drop the commit and force push if you don't like the approach. |
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
|
Sorry, this is still a long way from being prod-ready (at least not likely until end of the year) so it's probably not important to fix at this point. I'll revisit this before the release. |
Summary
With a slight modification to an import statement in react-fs it should
now support Node.js 12.
Test Plan
Start testing this module in Node.js 12