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

Decouple Metro config from CLI config #30

Merged
merged 33 commits into from
Dec 15, 2018
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
4396a67
Initial commit - delete Metro-related config
grabbou Dec 3, 2018
0995784
Rewrite of the config, part 2
grabbou Dec 3, 2018
b1ae80e
Finish adopting link & unlink to new config format
grabbou Dec 3, 2018
18c0e7b
Rewrite 'server' command and the way we load config
grabbou Dec 3, 2018
b3cce4e
Fix errors and make server run
grabbou Dec 3, 2018
e3f44c0
Use require.resolve
grabbou Dec 3, 2018
aba53ce
Simplify runServer code
grabbou Dec 3, 2018
e32d6c7
Updates to the config
grabbou Dec 3, 2018
6d3afbf
Remove duplicate platforms
grabbou Dec 3, 2018
6e21ad5
Use real and unified project root
grabbou Dec 3, 2018
c5d3e41
Fix projectRoots
grabbou Dec 3, 2018
9793d32
Unifiy root paths
grabbou Dec 3, 2018
cb76d47
Remove projectRoot from type
grabbou Dec 3, 2018
cf98b5a
Leave two todos for tomororw
grabbou Dec 3, 2018
15e43ba
Use projectRoot to read the root
grabbou Dec 3, 2018
038c190
Fix projectRoot and implement config again
grabbou Dec 3, 2018
213c130
Improve Metro config
grabbou Dec 4, 2018
ae258fd
Fix configuration again
grabbou Dec 4, 2018
372084a
Fix remaining Flow issues inside bundle
grabbou Dec 4, 2018
a67fe59
Fix two failing test suites
grabbou Dec 4, 2018
e2fa20b
Fix getDependencyConfig tests and use snapshots
grabbou Dec 4, 2018
46c9ae2
Remove console
grabbou Dec 4, 2018
b9ca8b7
Fix remaining Jest tests
grabbou Dec 4, 2018
3cb7bc6
Remove console.log
grabbou Dec 4, 2018
5bf9c71
Fix issues with link/unlink tested in production
grabbou Dec 4, 2018
40528b0
Add legacy config to avoid breaking changes
grabbou Dec 7, 2018
63fbef1
Fix a typo
grabbou Dec 7, 2018
1560725
Exclude React Native CLI from being linked
grabbou Dec 10, 2018
f20605e
Remove console.log from production code
grabbou Dec 10, 2018
7a67759
Update getCommands.js
grabbou Dec 15, 2018
da4136d
Update loadMetroConfig.js
grabbou Dec 15, 2018
4407e42
Update package.json
grabbou Dec 15, 2018
2925aa2
Update package.json
grabbou Dec 15, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 11 additions & 19 deletions packages/local-cli/bundle/buildBundle.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,28 +16,20 @@ const outputBundle = require('metro/src/shared/output/bundle');
const path = require('path');
const saveAssets = require('./saveAssets');

import type {RequestOptions, OutputOptions} from './types.flow';
import type {ConfigT} from 'metro-config/src/configTypes.flow';
const loadMetroConfig = require('../util/loadMetroConfig');

import type { ContextT } from '../core/types.flow';
import type { CommandLineArgs } from './bundleCommandLineArgs';

async function buildBundle(args: CommandLineArgs, ctx: ContextT, output = outputBundle) {
const config = await loadMetroConfig(ctx.root, {
resetCache: args.resetCache,
config: args.config
});

async function buildBundle(
args: OutputOptions & {
assetsDest: mixed,
entryFile: string,
maxWorkers: number,
resetCache: boolean,
transformer: string,
minify: boolean,
},
configPromise: Promise<ConfigT>,
/* $FlowFixMe(>=0.85.0 site=react_native_fb) This comment suppresses an error
* found when Flow v0.85 was deployed. To see the error, delete this comment
* and run Flow. */
output = outputBundle,
) {
// This is used by a bazillion of npm modules we don't control so we don't
// have other choice than defining it as an env variable here.
process.env.NODE_ENV = args.dev ? 'development' : 'production';
const config = await configPromise;

let sourceMapUrl = args.sourcemapOutput;
if (sourceMapUrl && !args.sourcemapUseAbsolutePath) {
Expand All @@ -52,7 +44,7 @@ async function buildBundle(
platform: args.platform,
};

const server = new Server({...config, resetCache: args.resetCache});
const server = new Server(config);

try {
const bundle = await output.build(server, requestOpts);
Expand Down
10 changes: 3 additions & 7 deletions packages/local-cli/bundle/bundle.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,15 @@ const outputBundle = require('metro/src/shared/output/bundle');
/**
* Builds the bundle starting to look for dependencies at the given entry path.
*/
function bundleWithOutput(argv, configPromise, args, output) {
if (!output) {
output = outputBundle;
}
return buildBundle(args, configPromise, output);
function bundleWithOutput(_, config, args, output) {
return buildBundle(args, config, output);
}

module.exports = {
name: 'bundle',
description: 'builds the javascript bundle for offline use',
func: bundleWithOutput,
options: bundleCommandLineArgs,

// not used by the CLI itself
// Used by `ramBundle.js`
withOutput: bundleWithOutput,
};
32 changes: 29 additions & 3 deletions packages/local-cli/bundle/bundleCommandLineArgs.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,32 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @format
* @flow
*/

'use strict';

const path = require('path');

export type CommandLineArgs = {
assetsDest?: string,
entryFile: string,
resetCache: boolean,
resetGlobalCache: boolean,
transformer?: string,
minify?: boolean,
config?: string,
platform?: string,
dev: boolean,
bundleOutput: string,
bundleEncoding?: string,
maxWorkers?: number,
sourcemapOutput?: string,
sourcemapSourcesRoot?: string,
sourcemapUseAbsolutePath: boolean,
verbose: boolean,
};

module.exports = [
{
command: '--entry-file <path>',
Expand All @@ -27,7 +48,7 @@ module.exports = [
{
command: '--dev [boolean]',
description: 'If false, warnings are disabled and the bundle is minified',
parse: val => (val === 'false' ? false : true),
parse: (val: string) => (val === 'false' ? false : true),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of (val: string) => (val === 'false' ? false : true), you could do !!JSON.parse(val)

default: true,
},
{
Expand All @@ -36,7 +57,7 @@ module.exports = [
'Allows overriding whether bundle is minified. This defaults to ' +
'false if dev is true, and true if dev is false. Disabling minification ' +
'can be useful for speeding up production builds for testing purposes.',
parse: val => (val === 'false' ? false : true),
parse: (val: string) => (val === 'false' ? false : true),
},
{
command: '--bundle-output <string>',
Expand Down Expand Up @@ -93,4 +114,9 @@ module.exports = [
'Try to fetch transformed JS code from the global cache, if configured.',
default: false,
},
{
command: '--config [string]',
description: 'Path to the CLI configuration file',
parse: (val: string) => path.resolve(val),
},
];
13 changes: 0 additions & 13 deletions packages/local-cli/bundle/types.flow.js

This file was deleted.

51 changes: 31 additions & 20 deletions packages/local-cli/cliEntry.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,25 +10,18 @@

'use strict';

const {configPromise} = require('./core');

const assertRequiredOptions = require('./util/assertRequiredOptions');
/* $FlowFixMe(>=0.54.0 site=react_native_oss) This comment suppresses an error
* found when Flow v0.54 was deployed. To see the error delete this comment and
* run Flow. */
const chalk = require('chalk');
const childProcess = require('child_process');
/* $FlowFixMe(>=0.54.0 site=react_native_oss) This comment suppresses an error
* found when Flow v0.54 was deployed. To see the error delete this comment and
* run Flow. */
const commander = require('commander');
const commands = require('./commands');
const getCommands = require('./core/getCommands');
const getLegacyConfig = require('./core/getLegacyConfig');
const minimist = require('minimist');
const init = require('./init/init');
const path = require('path');
const pkg = require('./package.json');

import type {CommandT} from './commands';
import type {RNConfig} from './core';
import type { CommandT, ContextT } from './core/types.flow';

commander.version(pkg.version);

Expand Down Expand Up @@ -99,7 +92,7 @@ function printUnknownCommand(cmdName) {
);
}

const addCommand = (command: CommandT, cfg: RNConfig) => {
const addCommand = (command: CommandT, ctx: ContextT) => {
const options = command.options || [];

const cmd = commander
Expand All @@ -114,38 +107,56 @@ const addCommand = (command: CommandT, cfg: RNConfig) => {
Promise.resolve()
.then(() => {
assertRequiredOptions(options, passedOptions);
return command.func(argv, cfg, passedOptions);
return command.func(argv, ctx, passedOptions);
})
.catch(handleError);
});

cmd.helpInformation = printHelpInformation.bind(cmd);
cmd.examples = command.examples;
// $FlowFixMe: This is either null or not
cmd.pkg = command.pkg;

options.forEach(opt =>
cmd.option(
opt.command,
opt.description,
opt.parse || defaultOptParser,
typeof opt.default === 'function' ? opt.default(cfg) : opt.default,
typeof opt.default === 'function' ? opt.default(ctx) : opt.default,
),
);

// Placeholder option for --config, which is parsed before any other option,
// but needs to be here to avoid "unknown option" errors when specified
cmd.option('--config [string]', 'Path to the CLI configuration file');

// This is needed to avoid `unknown option` error by Commander.js
cmd.option('--projectRoot [string]', 'Path to the root of the project');
};

async function run() {
const config = await configPromise;
const setupEnvScript = /^win/.test(process.platform)
? 'setup_env.bat'
: 'setup_env.sh';

childProcess.execFileSync(path.join(__dirname, setupEnvScript));

commands.forEach(cmd => addCommand(cmd, config));
/**
* Read passed `options` and take the "global" settings
*
* @todo(grabbou): Consider unifying this by removing either `commander`
* or `minimist`
*/
const options = minimist(process.argv.slice(2));

const root = options.projectRoot
? path.resolve(options.projectRoot)
: process.cwd();

const ctx = {
...getLegacyConfig(root),
root,
};

const commands = getCommands(ctx.root);

commands.forEach(command => addCommand(command, ctx));

commander.parse(process.argv);

Expand Down
79 changes: 0 additions & 79 deletions packages/local-cli/commands.js

This file was deleted.

20 changes: 0 additions & 20 deletions packages/local-cli/core/Constants.js

This file was deleted.

2 changes: 1 addition & 1 deletion packages/local-cli/core/__tests__/findAssets-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
jest.mock('path');
jest.mock('fs');

const findAssets = require('../findAssets');
const { findAssets } = require('../getAssets');
Copy link
Member

Choose a reason for hiding this comment

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

yo, your prettier config should not have these whitespaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, we don't have prettier set up in the repository just yet. I was waiting till we finish "migration" with that and then, I started this PR.

I was planning to use our own preset https://github.com/callstack/eslint-config-callstack as it already has prettier integration built into ESLint rules (it automatically configures itself based on ESLint settings).

That way we only need to run eslint --fix.

I would suggest to "temporarily" ignore styling issues (as long as they are not that "annoying") and I'll follow up with "prettier" PR right after this one is merged.

const dependencies = require('../__fixtures__/dependencies');
const fs = require('fs');

Expand Down
Loading