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

Added flow to babel cli #10219

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
d7714ad
added flow to directory cmd
letladi Jul 13, 2019
a0ec5e4
added flow to file cmd
letladi Jul 13, 2019
a254447
added flow to babel-cli utility file
letladi Jul 14, 2019
af50714
corrected errors in babel-cli file command
letladi Jul 14, 2019
a122deb
added flow to babel-cli babel-external-helpers
letladi Jul 14, 2019
a16b3d4
added flow to babel-cli babel-external-helpers. Iteration #2
letladi Jul 15, 2019
3d484d3
changed check inside readStdin inside babel file command to accomodat…
Jul 15, 2019
de384ff
removed toString method inside readStdin of babel-cli file command
Jul 15, 2019
dbb53b4
fixed file command to only include flow changes
letladi Jul 15, 2019
2cd51e2
added SourceMapGenerator as an arg type to fromObject
letladi Jul 16, 2019
3f759bc
reverted to only using SourceMap inside fromObject method
letladi Jul 16, 2019
d97a1f7
went back to returning 'null' from the async function that compiles f…
Jul 17, 2019
6667330
Merge branch 'add-flow-to-babel-cli' of github.com:Letladi/babel into…
Jul 17, 2019
5e567d9
added null promise return type to method in file command
letladi Jul 18, 2019
c67e611
Change duplicate tests for @babel/highlight getChalk method (#10093)
letladi Jun 15, 2019
6d2f111
flow - allow type parameter defaults in function declarations (#10084)
tanhauhau Jun 15, 2019
a4889a4
added flow to directory cmd
letladi Jul 13, 2019
01e08c1
added flow to file cmd
letladi Jul 13, 2019
f569385
added flow to babel-cli utility file
letladi Jul 14, 2019
3a680ac
corrected errors in babel-cli file command
letladi Jul 14, 2019
859bdaf
added flow to babel-cli babel-external-helpers
letladi Jul 14, 2019
ce1fc7b
added flow to babel-cli babel-external-helpers. Iteration #2
letladi Jul 15, 2019
ad6454d
changed check inside readStdin inside babel file command to accomodat…
Jul 15, 2019
daaae15
removed toString method inside readStdin of babel-cli file command
Jul 15, 2019
588ec1a
fixed file command to only include flow changes
letladi Jul 15, 2019
14859a3
added SourceMapGenerator as an arg type to fromObject
letladi Jul 16, 2019
6de54cc
reverted to only using SourceMap inside fromObject method
letladi Jul 16, 2019
ce351a1
added null promise return type to method in file command
letladi Jul 18, 2019
e20ce35
Merge branch 'add-flow-to-babel-cli' of github.com:Letladi/babel into…
Jul 19, 2019
3c31947
changed walk file Promise call to only an object promise
Jul 19, 2019
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
10 changes: 8 additions & 2 deletions packages/babel-cli/src/babel-external-helpers.js
@@ -1,9 +1,15 @@
// @flow

import commander from "commander";
import isString from "lodash/isString";
import { buildExternalHelpers } from "@babel/core";

function collect(value, previousValue): Array<string> {
function collect(
value: String | any,
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a string object, it's a string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes. Sorry, that is my mistake. Will fix

previousValue: Array<string>,
): Array<string> {
// If the user passed the option with no value, like "babel-external-helpers --whitelist", do nothing.
if (typeof value !== "string") return previousValue;
if (!isString(value)) return previousValue;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't 🙈
Will fix.


const values = value.split(",");

Expand Down
16 changes: 11 additions & 5 deletions packages/babel-cli/src/babel/dir.js
@@ -1,3 +1,5 @@
// @flow

import defaults from "lodash/defaults";
import outputFileSync from "output-file-sync";
import { sync as mkdirpSync } from "mkdirp";
Expand All @@ -6,11 +8,15 @@ import path from "path";
import fs from "fs";

import * as util from "./util";
import { type CmdOptions } from "./options";

export default async function({ cliOptions, babelOptions }) {
export default async function({
cliOptions,
babelOptions,
}: CmdOptions): Promise<void> {
const filenames = cliOptions.filenames;

async function write(src, base) {
async function write(src: string, base: string): Promise<boolean> {
let relative = path.relative(base, src);

if (!util.isCompilableExtension(relative, cliOptions.extensions)) {
Expand Down Expand Up @@ -65,14 +71,14 @@ export default async function({ cliOptions, babelOptions }) {
}
}

function getDest(filename, base) {
function getDest(filename: string, base: string): string {
if (cliOptions.relative) {
return path.join(base, cliOptions.outDir, filename);
}
return path.join(cliOptions.outDir, filename);
}

async function handleFile(src, base) {
async function handleFile(src: string, base: string): Promise<boolean> {
const written = await write(src, base);

if (!written && cliOptions.copyFiles) {
Expand All @@ -84,7 +90,7 @@ export default async function({ cliOptions, babelOptions }) {
return written;
}

async function handle(filenameOrDir) {
async function handle(filenameOrDir: string): Promise<number> {
if (!fs.existsSync(filenameOrDir)) return 0;

const stat = fs.statSync(filenameOrDir);
Expand Down
64 changes: 39 additions & 25 deletions packages/babel-cli/src/babel/file.js
@@ -1,15 +1,28 @@
// @flow

import convertSourceMap from "convert-source-map";
import defaults from "lodash/defaults";
import isString from "lodash/isString";
import toString from "lodash/toString";
import sourceMap from "source-map";
import slash from "slash";
import path from "path";
import fs from "fs";

import * as util from "./util";

export default async function({ cliOptions, babelOptions }) {
function buildResult(fileResults) {
const map = new sourceMap.SourceMapGenerator({
import { type CmdOptions } from "./options";

type CompilationOutput = {
code: string,
map: Object,
};

export default async function({
cliOptions,
babelOptions,
}: CmdOptions): Promise<void> {
function buildResult(fileResults: Array<Object>): CompilationOutput {
const map: Object = new sourceMap.SourceMapGenerator({
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't flow already infer this type to the SourceMapGenerator class? (defined at

declare class SourceMapGenerator {
)

Copy link
Contributor Author

@letladi letladi Jul 15, 2019

Choose a reason for hiding this comment

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

It does, but the fromObject method expects a SourceMap but we pass it SourceMapGenerator and so causes errors, so maybe I should add SourceMap | SourceMapGenerator to that module declaration?

Copy link
Contributor Author

@letladi letladi Jul 15, 2019

Choose a reason for hiding this comment

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

I'm thinking something like this fromObject(obj: SourceMap | SourceMapGenerator). This works without errors.

Copy link
Member

Choose a reason for hiding this comment

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

I think that fromObject(obj: SourceMap) was correct, and it isn't intended to be called with a SourceMapGenerator object.
It should be .fromObject(map.toJSON()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. I will make that change.

file:
cliOptions.sourceMapTarget ||
path.basename(cliOptions.outFile || "") ||
Expand Down Expand Up @@ -74,7 +87,7 @@ export default async function({ cliOptions, babelOptions }) {
};
}

function output(fileResults) {
function output(fileResults: Array<string>): void {
const result = buildResult(fileResults);

if (cliOptions.outFile) {
Expand All @@ -91,25 +104,27 @@ export default async function({ cliOptions, babelOptions }) {
}
}

function readStdin() {
return new Promise((resolve, reject) => {
let code = "";
function readStdin(): Promise<string> {
return new Promise(
(resolve: Function, reject: Function): void => {
let code = "";

process.stdin.setEncoding("utf8");
process.stdin.setEncoding("utf8");

process.stdin.on("readable", function() {
const chunk = process.stdin.read();
if (chunk !== null) code += chunk;
});
process.stdin.on("readable", function() {
const chunk = process.stdin.read();
if (isString(chunk)) code += toString(chunk);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need isString and toString here.

Note that chunk could also be a Buffer (docs), so isString(chunk) is not equivalent to chunk !== null.

code += chunk will implicitly convert chunk to string since code is a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove isString, but without toString flow will throw an error because it sees Buffer being incompatible with String

Copy link
Member

Choose a reason for hiding this comment

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

We can // $FlowIgnore it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright.

});

process.stdin.on("end", function() {
resolve(code);
});
process.stdin.on("error", reject);
});
process.stdin.on("end", function() {
resolve(code);
});
process.stdin.on("error", reject);
},
);
}

async function stdin() {
async function stdin(): Promise<void> {
const code = await readStdin();

const res = await util.transform(
Expand All @@ -126,7 +141,7 @@ export default async function({ cliOptions, babelOptions }) {
output([res]);
}

async function walk(filenames) {
async function walk(filenames: Array<string>): Promise<void> {
const _filenames = [];

filenames.forEach(function(filename) {
Expand All @@ -151,7 +166,7 @@ export default async function({ cliOptions, babelOptions }) {
});

const results = await Promise.all(
_filenames.map(async function(filename) {
_filenames.map(async function(filename: string): Promise<Object> {
let sourceFilename = filename;
if (cliOptions.outFile) {
sourceFilename = path.relative(
Expand All @@ -168,7 +183,7 @@ export default async function({ cliOptions, babelOptions }) {
{
sourceFileName: sourceFilename,
// Since we're compiling everything to be merged together,
// "inline" applies to the final output file, but to the individual
// "inline" applies to the final output file, but not to the individual
// files being concatenated.
sourceMaps:
babelOptions.sourceMaps === "inline"
Expand All @@ -184,15 +199,14 @@ export default async function({ cliOptions, babelOptions }) {
}

console.error(err);
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest leave return null here so that we can make this PR focus on adding flow type annotation without changing any internal logic.

Note that you can use union type to specify this function returns either a Promise<Object> or null.

_filenames.map(async function(filename: string): Promise<Object> | null {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll make the fix.

}
}),
);

output(results);
}

async function files(filenames) {
async function files(filenames: Array<string>): Promise<void> {
if (!cliOptions.skipInitialBuild) {
await walk(filenames);
}
Expand All @@ -208,7 +222,7 @@ export default async function({ cliOptions, babelOptions }) {
pollInterval: 10,
},
})
.on("all", function(type, filename) {
.on("all", function(type: string, filename: string) {
if (!util.isCompilableExtension(filename, cliOptions.extensions)) {
return;
}
Expand Down
9 changes: 8 additions & 1 deletion packages/babel-cli/src/babel/options.js
@@ -1,3 +1,5 @@
// @flow

import fs from "fs";

import commander from "commander";
Expand Down Expand Up @@ -151,7 +153,12 @@ commander.option(
commander.version(pkg.version + " (@babel/core " + version + ")");
commander.usage("[options] <files ...>");

export default function parseArgv(args: Array<string>) {
export type CmdOptions = {
babelOptions: Object,
cliOptions: Object,
};

export default function parseArgv(args: Array<string>): CmdOptions {
//
commander.parse(args);

Expand Down
32 changes: 22 additions & 10 deletions packages/babel-cli/src/babel/util.js
@@ -1,10 +1,12 @@
// @flow

import readdirRecursive from "fs-readdir-recursive";
import * as babel from "@babel/core";
import includes from "lodash/includes";
import path from "path";
import fs from "fs";

export function chmod(src, dest) {
export function chmod(src: string, dest: string): void {
fs.chmodSync(dest, fs.statSync(src).mode);
}

Expand All @@ -13,8 +15,8 @@ type ReaddirFilter = (filename: string) => boolean;
export function readdir(
dirname: string,
includeDotfiles: boolean,
filter: ReaddirFilter,
) {
filter?: ReaddirFilter,
): Array<string> {
return readdirRecursive(dirname, (filename, _index, currentDirectory) => {
const stat = fs.statSync(path.join(currentDirectory, filename));

Expand All @@ -30,7 +32,7 @@ export function readdirForCompilable(
dirname: string,
includeDotfiles: boolean,
altExts?: Array<string>,
) {
): Array<string> {
return readdir(dirname, includeDotfiles, function(filename) {
return isCompilableExtension(filename, altExts);
});
Expand All @@ -48,15 +50,19 @@ export function isCompilableExtension(
return includes(exts, ext);
}

export function addSourceMappingUrl(code, loc) {
export function addSourceMappingUrl(code: string, loc: string): string {
return code + "\n//# sourceMappingURL=" + path.basename(loc);
}

const CALLER = {
name: "@babel/cli",
};

export function transform(filename, code, opts) {
export function transform(
filename: string,
code: string,
opts: Object,
): Promise<Object> {
opts = {
...opts,
caller: CALLER,
Expand All @@ -71,7 +77,10 @@ export function transform(filename, code, opts) {
});
}

export function compile(filename, opts) {
export function compile(
filename: string,
opts: Object | Function,
): Promise<Object> {
opts = {
...opts,
caller: CALLER,
Expand All @@ -85,7 +94,7 @@ export function compile(filename, opts) {
});
}

export function deleteDir(path) {
export function deleteDir(path: string): void {
if (fs.existsSync(path)) {
fs.readdirSync(path).forEach(function(file) {
const curPath = path + "/" + file;
Expand All @@ -106,7 +115,7 @@ process.on("uncaughtException", function(err) {
process.exit(1);
});

export function requireChokidar() {
export function requireChokidar(): Object {
try {
return require("chokidar");
} catch (err) {
Expand All @@ -118,7 +127,10 @@ export function requireChokidar() {
}
}

export function adjustRelative(relative, keepFileExtension) {
export function adjustRelative(
relative: string,
keepFileExtension: boolean,
): string {
if (keepFileExtension) {
return relative;
}
Expand Down