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

feature: New command 'set' and 'get' to play with the cli config. #407

Merged
merged 1 commit into from
Apr 14, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 12 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
node_modules/
.idea
jsconfig.json
npm-debug.log

# IDEs
.idea/
jsconfig.json

# Typings file.
typings/

# Misc
tmp/

# Mac OSX Finder files.
**/.DS_Store
.DS_Store
29 changes: 29 additions & 0 deletions addon/ng2/commands/get.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import * as chalk from 'chalk';
import * as Command from 'ember-cli/lib/models/command';
import {CliConfig} from '../models/config';


const GetCommand = Command.extend({
name: 'get',
description: 'Set a value in the configuration.',
works: 'outsideProject',

availableOptions: [],
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't there be a global option ?


run: function (commandOptions, rawArgs): Promise<void> {
return new Promise(resolve => {
const value = new CliConfig().get(rawArgs[0]);
if (value === null) {
console.error(chalk.red('Value cannot be found.'));
} else if (typeof value == 'object') {
Copy link
Contributor

Choose a reason for hiding this comment

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

== should be ===

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typeof X will always be a string.

Copy link
Member

Choose a reason for hiding this comment

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

typeof X will always be a string.

OOC, is thete any reason not to use === then ?

console.log(JSON.stringify(value));
} else {
console.log(value);
}
resolve();
});
}
});

module.exports = GetCommand;
module.exports.overrideCore = true;
25 changes: 25 additions & 0 deletions addon/ng2/commands/set.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import * as Command from 'ember-cli/lib/models/command';
import {CliConfig} from '../models/config';


const SetCommand = Command.extend({
name: 'set',
description: 'Set a value in the configuration.',
works: 'outsideProject',

availableOptions: [
{ name: 'global', type: Boolean, default: false, aliases: ['g'] },
],

run: function (commandOptions, rawArgs): Promise<void> {
return new Promise(resolve => {
const config = new CliConfig();
config.set(rawArgs[0], rawArgs[1], commandOptions.force);
config.save();
resolve();
});
}
});

module.exports = SetCommand;
module.exports.overrideCore = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to override this command as it doesn't exists in ember-cli. That also stands for get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah 👍 thanks.

6 changes: 5 additions & 1 deletion addon/ng2/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ module.exports = {
'version': require('./commands/version'),
'completion': require('./commands/completion'),
'doc': require('./commands/doc'),
'github-pages-deploy': require('./commands/github-pages-deploy')
'github-pages-deploy': require('./commands/github-pages-deploy'),

// Configuration
Copy link
Contributor

@jkuri jkuri Apr 14, 2016

Choose a reason for hiding this comment

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

Do we need comment and break line here?

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's just to split it. We can do it, so why not do make it clearer?

'set': require('./commands/set'),
'get': require('./commands/get')
};
}
};
125 changes: 125 additions & 0 deletions addon/ng2/models/config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
import * as fs from 'fs';
import * as path from 'path';


export const CLI_CONFIG_FILE_NAME = 'angular-cli.json';


export interface CliConfigJson {
routes?: { [name: string]: any },
packages?: { [name: string]: any }
}


function _findUp(name: string, from: string) {
let currentDir = from;
while (currentDir && currentDir != '/') {
const p = path.join(currentDir, name);
if (fs.existsSync(p)) {
return p;
}

currentDir = path.resolve(currentDir, '..');
}

return null;
}


export class CliConfig {
constructor(private _config: CliConfigJson = CliConfig.fromProject()) {}

save(path: string = CliConfig.configFilePath()) {
if (!path) {
throw new Error('Could not find config path.');
}

fs.writeFileSync(path, JSON.stringify(this._config, null, 2), { encoding: 'utf-8' });
}

set(jsonPath: string, value: any, force: boolean = false): boolean {
let { parent, name, remaining } = this._findParent(jsonPath);
while (force && remaining) {
if (remaining.indexOf('.') != -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

!= should be !==

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indexOf() will always return a number.

I don't think we should be super strict about === unless there is a case for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose in TS it isn't as relevant, I'm just used to the strictness from normal JS.

// Create an empty map.
// TODO: create the object / array based on the Schema of the configuration.
parent[name] = {};
}

}

parent[name] = value;
return true;
}

get(jsonPath: string): any {
let { parent, name, remaining } = this._findParent(jsonPath);
if (remaining || !(name in parent)) {
return null;
} else {
return parent[name];
}
}

private _validatePath(jsonPath: string) {
if (!jsonPath.match(/^(?:[-_\w\d]+(?:\[\d+\])*\.)*(?:[-_\w\d]+(?:\[\d+\])*)$/)) {
throw `Invalid JSON path: "${jsonPath}"`;
}
}

private _findParent(jsonPath: string): { parent: any, name: string | number, remaining?: string } {
this._validatePath(jsonPath);

let parent: any = null;
let current: any = this._config;

const splitPath = jsonPath.split('.');
let name: string | number = '';

while (splitPath.length > 0) {
const m = splitPath.shift().match(/^(.*?)(?:\[(\d+)\])*$/);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does m stand for?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm, I suppose it's match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup.


name = m[1];
const index: string = m[2];
parent = current;
current = current[name];

if (current === null || current === undefined) {
return {
parent,
name,
remaining: (!isNaN(index) ? `[${index}]` : '') + splitPath.join('.')
};
}

if (!isNaN(index)) {
name = index;
parent = current;
current = current[index];

if (current === null || current === undefined) {
return {
parent,
name,
remaining: splitPath.join('.')
};
}
}
}

return { parent, name };
}

static configFilePath(projectPath?: string): string {
// Find the configuration, either where specified, in the angular-cli project
// (if it's in node_modules) or from the current process.
return (projectPath && _findUp(CLI_CONFIG_FILE_NAME, projectPath))
|| _findUp(CLI_CONFIG_FILE_NAME, __dirname)
|| _findUp(CLI_CONFIG_FILE_NAME, process.cwd());
}

static fromProject(): CliConfigJson {
const configPath = this.configFilePath();
return configPath ? require(configPath) : {};
}
}
139 changes: 139 additions & 0 deletions docs/design/ngConfig.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
# ngConfig - Design

## Goals

Currently, a project scaffolded with the CLI have no way of specifying options and configurations affecting their projects. There are ways to affect the build (with the `angular-cli-build.js` file), but the following questions cannot be answered without actual project options:

* Where in my directory is my karma.conf file?
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be separated questions?

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's just something that should be an option.

* What is my firebase database URL?
* Where is my client code?
* How can I use a different lazy-loading boundary prefix (or none at all)?
* Any other backend I want to run prior to `ng serve`?

# Proposed Solution

Since the data is static, we only need to keep it in a static store somewhere.

One solution would be to keep the data in the `package.json`. Unfortunately, the metadata contains too much data and the `package.json` file would become unmanageable.

Instead of polluting the package file, a `angular-cli.json` file will be created that contains all the values. Access to that file will be allowed to the user if he knows the structure of the file (unknown keys will be kept but ignored), and it's easy to read and write. We can also keep the file formatted.


## Fallback

There should be two `angular-cli.json` files; one for the project and a general one. The general one should contain information that can be useful when scaffolding new apps, or informations about the user.

The project `angular-cli.json` goes into the project root. The global configuration should live at `$HOME/.angular-cli.json`.

## Structure

The structure should be defined by a JSON schema (see [here](http://json-schema.org/)). The schema will be used to generate the `d.ts`, but that file will be kept in the file system along the schema for IDEs.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this d.ts and where is 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.

It lives next to the JSON schema. We will need both, unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the d.ts used for? I'm sorry, I don't really understand this bit. Is a requirement to work with the json-schema or something?

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 contains the interface for the schema.

Suppose a schema:

{
    "title": "Example Schema",
    "type": "object",
    "properties": {
        "firstName": {
            "type": "string"
        },
        "lastName": {
            "type": "string"
        },
        "age": {
            "description": "Age in years",
            "type": "integer",
            "minimum": 0
        }
    },
    "required": ["firstName", "lastName"]
}

We create an interface like:

interface CliConfig {
  firstName: string;
  lastName: string;
  age?: number;
}

You can import that interface in TypeScript and the IDE will help you with autocorrect, completion, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I think I get it. So this would help us in the CLI when importing the config directly.

But that means there are cases when we should use the model to interact with the config, and cases where we should interact directly using this interface to help us, right? What scenarios should we not use the model?

Also, I know it's not the real name but perhaps it shouldn't be CliConfig, since the model class is also named the same.


Every PR that would change the schema should include the update to the `d.ts`.

# API

## CLI

#### Getting values
Copy link
Contributor

Choose a reason for hiding this comment

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

When getting values from within the CLI the options object should be exposed via the already existing project object. This will have the benefit of not having to require/import in the configuration object from many different places within the CLI. From commands/tasks/generate blueprints, I would like to see the ability to do the following:

var mySetting = this.project.ng.mySetting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


The new command `get` should be used to output values on the terminal. It takes a set of flags and an optional array of [paths](#path);

* `--glob` or `-g`; the path follows a glob format, where `*` can be replaced by any amount of characters and `?` by a single character. This will output `name=value` for each values matched.

Otherwise, outputs the value of the path passed in. If multiple paths are passed in, they follow the format of `name=value`.

#### Setting values

The new command `set` should be used to set values in the local configuration file. It takes a set of flags and an optional array of `[path](#path)=value`;

* `--global`; sets the value in the global configuration.
* `--remove`; removes the key (no value should be passed in).

The schema needs to be taken into account when setting the value of the field;

* If the field is a number, the string received from the command line is parsed. `NaN` throws an error.
* If the field is an object, an error is thrown.
* If the path is inside an object but the object hasn't been defined yet, sets the object with empty values (use the schema to create a valid object).

#### Path<a name="path"></a>

The paths are json formatted path; each `.` separates a map, while `[]` indicates an index in an array.

An example is the following:

keyA.keyB.arrayC[3].value

## Model

A model should be created that will include loading and saving the configuration, including the global configuration.

**The model should be part of the project and created on the `project` object.**

That model can be used internally by the tool to get information. It will include a proxy handler that throws if an operation doesn't respect the schema. It will also sets values on globals and locals depending on which branches you access.

A simple API would return the TypeScript interface:

```typescript
class Config {
// ...
get local(): ICliConfig { /* ... */ }
get global(): ICliConfig { /* ... */ }
}
```

The `local` and `global` getters return proxies that respect the JSON Schema defined for the Angular config. These proxies allow users to not worry about the existence of values; those values will only be created on disc when they are setted.

Also, `local` will always defer to the same key-path in `global` if a value isn't available. If a value is set and the parent exists in `global`, it should be created to `local` such that it's saved locally to the project. The proxies only care about the end points of `local` and `global`, not the existence of a parent in either.

For example, assuming the following globals/locals:

```js
// Global
{
"key1": {
"key2": {
"value": 0,
"value2": 1
}
}
}

// Local
{
"key1": {
"key2": {
"value2": 2,
"value3": 3
}
}
}
```

The following stands true:

```typescript
const config = new Config(/* ... */);

console.log(config.local.key1.key2.value); // 0, even if it doesn't exist.
console.log(config.local.key1.key2.value2); // 2, local overrides.
console.log(config.local.key1.key2.value3); // 3.
console.log(config.local.key1.key2.value4); // Schema's default value.

console.log(config.global.key1.key2.value); // 0.
console.log(config.global.key1.key2.value2); // 1, only global.
console.log(config.global.key1.key2.value3); // Schema's default value.

config.local.key1.key2.value = 1;
// Now the value is 1 inside the local. Global stays the same.

config.local.key1.key2.value3 = 5;
// The global config isn't modified.

config.global.key1.key2.value4 = 99;
// The local config stays the same.
console.log(config.local.key1.key2.value4); // 99, the global value.

config.save(); // Commits if there's a change to global and/or local.
```

17 changes: 17 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"compilerOptions": {
"declaration": true,
"emitDecoratorMetadata": true,
"experimentalDecorators": true,
"mapRoot": "",
"module": "commonjs",
"moduleResolution": "node",
"noEmitOnError": true,
"noImplicitAny": true,
"outDir": "../dist/",
"rootDir": ".",
"sourceMap": true,
"sourceRoot": "/",
"target": "es5"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We updated the tsconfig.json to use inline sources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for the whole project, not the scaffolds :)