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.

Sign up for GitHub

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

Jump to bottom

Add Node.js 12 support to react-fs #20541

Closed
wants to merge 5 commits into from
Closed

Add Node.js 12 support to react-fs #20541

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
138d9cb
Add Node.js 12 support to react-fs
MylesBorins Jan 4, 2021
847841d
fixup! Add Node.js 12 support to react-fs
MylesBorins Jan 4, 2021
9d5d328
fixup! fixup! Add Node.js 12 support to react-fs
MylesBorins Jan 4, 2021
9c70305
fixup! fixup! fixup! Add Node.js 12 support to react-fs
MylesBorins Jan 4, 2021
f49efc8
fixup! fixup! fixup! fixup! Add Node.js 12 support to react-fs
MylesBorins Jan 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-fs/src/ReactFilesystem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import type {Wakeable, Thenable} from 'shared/ReactTypes';

import {unstable_getCacheForType} from 'react';
import * as fs from 'fs/promises';
import {promises as fs} from 'fs';
import {isAbsolute, normalize} from 'path';

const Pending = 0;
Expand Down
101 changes: 62 additions & 39 deletions scripts/flow/environment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,50 +70,73 @@ declare module 'EventListener' {
declare function __webpack_chunk_load__(id: string): Promise<mixed>;
declare function __webpack_require__(id: string): any;

declare module 'fs/promises' {
declare var access: (path: string, mode?: number) => Promise<void>;
declare var lstat: (
path: string,
options?: ?{bigint?: boolean},
) => Promise<mixed>;
declare var readdir: (
path: string,
declare module 'fs' {
declare function existsSync(path: string): boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

declare function readFileSync(path: string | Buffer | URL | number): Buffer;
declare function readFileSync(
path: string | Buffer | URL | number,
encoding: string,
): string;
declare function readFileSync(
path: string | Buffer | URL | number,
options: {encoding: string, flag?: string},
): string;
declare function readFileSync(
path: string | Buffer | URL | number,
options: {encoding?: void, flag?: string},
): Buffer;
declare function writeFileSync(
filename: string,
data: Buffer | string,
options?:
| ?string
| string
| {
encoding?: ?string,
withFileTypes?: ?boolean,
mode?: number,
flag?: string,
},
) => Promise<Buffer>;
declare var readFile: (
path: string,
options?:
| ?string
| {
encoding?: ?string,
},
) => Promise<Buffer>;
declare var readlink: (
path: string,
options?:
| ?string
| {
encoding?: ?string,
},
) => Promise<mixed>;
declare var realpath: (
path: string,
options?:
| ?string
| {
encoding?: ?string,
},
) => Promise<mixed>;
declare var stat: (
path: string,
options?: ?{bigint?: boolean},
) => Promise<mixed>;
): void;
declare class FSPromise {
access(path: string, mode?: number): Promise<void>;
lstat(path: string, options?: ?{bigint?: boolean}): Promise<mixed>;
readdir(
path: string,
options?:
| ?string
| {
encoding?: ?string,
withFileTypes?: ?boolean,
},
): Promise<Buffer>;
readFile(
path: string,
options?:
| ?string
| {
encoding?: ?string,
},
): Promise<Buffer>;
readlink(
path: string,
options?:
| ?string
| {
encoding?: ?string,
},
): Promise<mixed>;
realpath(
path: string,
options?:
| ?string
| {
encoding?: ?string,
},
): Promise<mixed>;
stat(path: string, options?: ?{bigint?: boolean}): Promise<mixed>;
}
declare var promises: FSPromise;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can put the type here inline without a class?

declare var promises: {
  // ...
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern was copied directly from the default type definitions

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/facebook/flow/blob/master/lib/node.js#L1354

}

declare module 'pg' {
declare var Pool: (
options: mixed,
Expand Down
2 changes: 1 addition & 1 deletion scripts/rollup/bundles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ const bundles = [
moduleType: ISOMORPHIC,
entry: 'react-fs/index.node.server',
global: 'ReactFilesystem',
externals: ['react', 'fs/promises', 'path'],
externals: ['react', 'fs', 'path'],
},

/******* React PG Browser (experimental, new) *******/
Expand Down
1 change: 0 additions & 1 deletion scripts/rollup/modules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ const HAS_NO_SIDE_EFFECTS_ON_IMPORT = false;
// const HAS_SIDE_EFFECTS_ON_IMPORT = true;
const importSideEffects = Object.freeze({
fs: HAS_NO_SIDE_EFFECTS_ON_IMPORT,
'fs/promises': HAS_NO_SIDE_EFFECTS_ON_IMPORT,
path: HAS_NO_SIDE_EFFECTS_ON_IMPORT,
'prop-types/checkPropTypes': HAS_NO_SIDE_EFFECTS_ON_IMPORT,
'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface': HAS_NO_SIDE_EFFECTS_ON_IMPORT,
Expand Down