Skip to content

feat(create-cycle-app): support motorcycle and ava #45

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

Closed
wants to merge 2 commits into from
Closed

feat(create-cycle-app): support motorcycle and ava #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16db788
feat(create-cycle-app): support motorcycle and ava
kristianmandrup Nov 28, 2016
fea5640
fix(create-cycle-app): Fix lint errors
kristianmandrup Nov 28, 2016
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
11 changes: 11 additions & 0 deletions create-cycle-app/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
# v2.1.0 (2016-11-28)
Copy link
Member

Choose a reason for hiding this comment

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

There isn't the need to change the CHANGELOG.md (as counter-intuitive as this seems), because it is updated by the release pipeline. So please, remove this change.

---
By: kmandrup@gmail.com

- Allow selection of core cycle library: cycle or motorcycle
- Allow selection of test runner: mocha or ava
- Add immutable and @cycle/isolate as basic dependencies
- Make generator infrastructure more extensible by:
- passing libs object
- use EJS templating to generate initial test file

# v2.0.0 (2016-11-01)
---

Expand Down
60 changes: 52 additions & 8 deletions create-cycle-app/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,48 @@ function createApp (name, verbose, flavor) {
}
}

var testLibQuestion = {
name: 'testLib',
message: 'Which test library do you want to use?',
type: 'list',
choices: [
{
name: "Good ol' Mocha",
value: 'mocha'
},
{
name: 'Ava, the Futuristic JavaScript test runner',
value: 'ava'
}
]
}

var cycleCoreLib = {
name: 'cycleLib',
message: 'Which cycle core library do you want to use?',
type: 'list',
choices: [
{
name: 'A basic Cycle',
value: 'cycle'
},
{
name: 'A super powerful Motorcycle',
value: 'motorcycle'
}
]
}

if (flavor) {
// Ask just for the stream library
inquirer.prompt([streamLibQuestion]).then(function (answers) {
preparePackageJson(appFolder, appName, flavor, answers.streamLib, verbose)
inquirer.prompt([cycleCoreLib, streamLibQuestion, testLibQuestion]).then(function (answers) {
Copy link
Member

Choose a reason for hiding this comment

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

I liked this idea, but considering the philosophy of CCA (one of the key points is the focus for beginners), we prefer to keep things simple and small, so, I'll ask you to put this changes around test library choices in another PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

mmm, although a nice idea testLibQuestion I'm not sure I see this as something that should be part of CCA as is not an option related to cycle itself (while the others definitely support diversity)

Copy link

Choose a reason for hiding this comment

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

Then why have a test lib at all? where to draw the line?

Copy link
Member

Choose a reason for hiding this comment

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

It's a very good question. Considering recent efforts made by @Widdershin on https://github.com/cyclejs/time, we could consider expanding the default tests examples, showing the basics of how to test cycle components (at least for the current "Hello world" component)

var libs = {
stream: answers.streamLib,
test: answers.testLib,
cycle: answers.cycleLib
}

preparePackageJson(appFolder, appName, flavor, libs, verbose)
})
} else {
inquirer.prompt([
Expand All @@ -103,6 +141,12 @@ function createApp (name, verbose, flavor) {
},
streamLibQuestion
]).then(function (answers) {
var libs = {
stream: answers.streamLib,
test: answers.testLib,
cycle: answers.cycleLib
}

// When run-discovery was choosed, we need to discover
// flavors somewhere else
if (answers.flavor === 'run-discovery') {
Expand All @@ -120,11 +164,11 @@ function createApp (name, verbose, flavor) {
},
streamLibQuestion
]).then(function (answers) {
preparePackageJson(appFolder, appName, answers.flavor, answers.streamLib, verbose)
preparePackageJson(appFolder, appName, answers.flavor, libs, verbose)
})
})
} else {
preparePackageJson(appFolder, appName, answers.flavor, answers.streamLib, verbose)
preparePackageJson(appFolder, appName, answers.flavor, libs, verbose)
}
})
}
Expand Down Expand Up @@ -174,7 +218,7 @@ function discoverFlavors (cb) {
})
}

function preparePackageJson (appFolder, appName, flavor, streamLib, verbose) {
function preparePackageJson (appFolder, appName, flavor, libs, verbose) {
// Start creating the new app
console.log(chalk.green('Creating a new Cycle.js app in ' + appFolder + '.'))
console.log()
Expand All @@ -190,11 +234,11 @@ function preparePackageJson (appFolder, appName, flavor, streamLib, verbose) {
JSON.stringify(packageJson, null, 2)
)

installScripts(appFolder, appName, flavor, streamLib, verbose)
installScripts(appFolder, appName, flavor, libs, verbose)
}

// Install and init scripts
function installScripts (appFolder, appName, flavor, streamLib, verbose) {
function installScripts (appFolder, appName, flavor, libs, verbose) {
var originalDirectory = process.cwd()
process.chdir(appFolder)

Expand Down Expand Up @@ -236,7 +280,7 @@ function installScripts (appFolder, appName, flavor, streamLib, verbose) {
var init = require(initScriptPath)

// Execute the cycle-scripts's specific initialization
init(appFolder, appName, streamLib, verbose, originalDirectory)
init(appFolder, appName, libs.stream, verbose, originalDirectory)
})
}

Expand Down
2 changes: 1 addition & 1 deletion create-cycle-app/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "create-cycle-app",
"version": "2.0.0",
"version": "2.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

There isn't need to change the number, as release pipeline update it too.

"description": "Create Cycle.js with no build configuration.",
"scripts": {
"test": "echo \"Maybe later\""
Expand Down
2 changes: 2 additions & 0 deletions cycle-scripts-es-browserify/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"cycle-scripts": "./index.js"
},
"dependencies": {
"ava": "^0.17",
Copy link
Member

Choose a reason for hiding this comment

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

So, I think that you are hardcoding ava as a dependency, like mocha. As your proposal is to make this configurable, we should consider remove the hard-dependency and make them peerDependencies, and install the lib choosed by the user together with other dependencies.

Copy link

Choose a reason for hiding this comment

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

Because I saw no other easy way to add ava with current very inflexible architecture, besides creating a set of new duplicate projects for all combinations. Hardly the way to go about it.

Copy link
Member

@geovanisouza92 geovanisouza92 Dec 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

You see inflexibility where I see openness. There isn't the need to duplicate each core flavor, because you could create new ones and publish them on npm independently.

Each cycle-scripts-* you see here, could be copied into motorcycle-scripts-* (or forked) and customized for motorcycle (you could just ignore cycle-specific options on init script). There you could change test libs, template files and everything else you see suitable as motorcycle defaults.

I'm not rejecting changes on the tool, I'm just arguing that there's easier ways to achieve the same results without bloating the core tool and flavors.

Ok, so, I see 4 valuable proposals until now:

  • Including @cycle/isolate as a default dependency
  • Adding motorcycle flavors
  • Making the test library/tool an option
  • Templating files

My proposal is that we turn each of this points into issues and build them progressively. What do you think?

"babel-preset-es2015": "^6.16.0",
"babel-register": "^6.16.3",
"babelify": "^7.3.0",
Expand All @@ -27,6 +28,7 @@
"chalk": "^1.1.3",
"cross-spawn": "^4.0.2",
"dotenv": "^2.0.0",
"ejs": "^2.5.2",
Copy link
Member

Choose a reason for hiding this comment

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

I cannot agree with this in any manner. Installing a devDependency on the user's project just to render one example test file?

No, I think that this could be solved with two files and a single if inside init script.

Copy link

Choose a reason for hiding this comment

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

I didn't know how else to add it. I didn't quite understand your nested projects infrastructure. Maybe you could document how to add the tools to do the work?
I've never seen anything quite like it. Loads of duplication to maintain in this way...
I always just have a single set of project templates... I developed dozens of similar generators in the past and never had to resort to this.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that adding some more contribution docs would be really helpful.

BTW there is one question to be made: "Is this dependency used only to create the project or it's a project dependency itself?"

When is the first case, we should find simple (hacky) ways to do it on init scripts. Otherwise, it's a dependency.

Each flavor' dependency becomes a devDependency when the project is "ejected", so, if the dependency would be put in devDependencies on a normal project, it must be on flavor dependencies.

"envify": "^3.4.1",
"exorcist": "^0.4.0",
"fs-extra": "^0.30.0",
Expand Down
65 changes: 52 additions & 13 deletions cycle-scripts-es-browserify/scripts/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,19 @@ var fs = require('fs-extra')
var path = require('path')
var spawn = require('cross-spawn')
var chalk = require('chalk')
var ejs = require('ejs')

function dependencies (streamLib) {
var basicDependencies = [
'@cycle/dom',
'xstream'
]
function dependencies (libs) {
var basicDependencies = []
var core = {
cycle: ['@cycle/dom', 'xstream'],
motorcycle: ['@motorcycle/core', '@motorcycle/dom']
}
var extras = ['immutable', '@cycle/isolate']
Copy link
Member

Choose a reason for hiding this comment

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

So, what's the point in installing immutable by default? @cycle/isolate seems to be a recurrent case, so, this could be included on all flavors in a proper PR, but immutable seems too opinionated to be in the core flavors.

Usually I prefer to use ramda, that is a totally different lib, but serves me for similar cases. But again, this is my preference, not the end-user.

Copy link
Collaborator

Choose a reason for hiding this comment

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

💯 on including @cycle/isolate .
Agree that immutable should still a user choice and a default of a core-flavor

Copy link

Choose a reason for hiding this comment

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

Good


basicDependencies.concat(core[libs.cycle], extras)

switch (streamLib) {
switch (libs.stream) {
case 'xstream':
return basicDependencies.concat(['@cycle/xstream-run'])
case 'most':
Expand All @@ -21,12 +26,12 @@ function dependencies (streamLib) {
case 'rx':
return basicDependencies.concat(['@cycle/rx-run', 'rx'])
default:
throw new Error('Unsupported stream library: ' + streamLib)
throw new Error('Unsupported stream library: ' + libs.stream)
}
}

function replacements (streamLib) {
switch (streamLib) {
function replacements (libs) {
switch (libs.stream) {
case 'xstream':
return {
'--RUN-LIB--': '@cycle/xstream-run',
Expand All @@ -52,7 +57,7 @@ function replacements (streamLib) {
'--STREAM--': 'Rx.Observable'
}
default:
throw new Error('Unsupported stream library: ' + streamLib)
throw new Error('Unsupported stream library: ' + libs.stream)
}
}

Expand Down Expand Up @@ -101,6 +106,18 @@ function patchAppJs (appPath, tags) {
)
}

function patchTestJs (appPath, testLib) {
var testJsPath = path.join(appPath, 'src', 'app.test.js')

var templateContent = fs.readFileSync(testJsPath, {encoding: 'utf-8'})

var testContent = ejs.compile(templateContent).render({test: testLib})
fs.writeFileSync(
testJsPath,
testContent
)
}

function successMsg (appName, appPath) {
console.log()
console.log('Success! Created ' + appName + ' at ' + appPath)
Expand Down Expand Up @@ -131,12 +148,33 @@ function successMsg (appName, appPath) {
console.log('Happy cycling!')
}

module.exports = function (appPath, appName, streamLib, verbose, originalDirectory) {
module.exports = function (appPath, appName, libs, verbose, originalDirectory) {
var ownPackageName = require(path.join(__dirname, '..', 'package.json')).name
var ownPath = path.join(appPath, 'node_modules', ownPackageName)
var appPackageJson = path.join(appPath, 'package.json')
var appPackage = require(appPackageJson)

if (libs.test === 'ava') {
appPackage.ava = {
Copy link
Member

Choose a reason for hiding this comment

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

There is another way to pass this parameters to the test runner? If possible, we should avoid polluting package.json with configuration that could be kept inside the cycle-scripts-* and put them only after eject.

files: [
'src/**/*.test.{js}',
'!dist/**/*'
],
source: [
'src/**/*.{js,jsx}',
'!dist/**/*'
],
concurrency: 5,
failFast: true,
tap: true,
powerAssert: true,
require: [
'babel-register'
],
babel: 'inherit'
}
}

// Manipulate app's package.json
appPackage.dependencies = appPackage.dependencies || {}
appPackage.devDependencies = appPackage.devDependencies || {}
Expand All @@ -157,8 +195,9 @@ module.exports = function (appPath, appName, streamLib, verbose, originalDirecto
// Copy flavor files
fs.copySync(path.join(ownPath, 'template'), appPath)

patchTestJs(appPath, libs.test)
patchGitignore(appPath)
var tags = replacements(streamLib)
var tags = replacements(libs)
patchIndexJs(appPath, tags)
patchAppJs(appPath, tags)

Expand All @@ -168,7 +207,7 @@ module.exports = function (appPath, appName, streamLib, verbose, originalDirecto
var args = [
'install'
].concat(
dependencies(streamLib) // Flavor dependencies
dependencies(libs) // Flavor dependencies
).concat([
'--save',
verbose && '--verbose'
Expand Down
2 changes: 2 additions & 0 deletions cycle-scripts-es-webpack/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@
"cycle-scripts": "./index.js"
},
"dependencies": {
"ava": "^0.17",
"babel-core": "^6.17.0",
"babel-loader": "^6.2.5",
"babel-preset-es2015": "^6.16.0",
"babel-register": "^6.16.3",
"chalk": "^1.1.3",
"cross-spawn": "^4.0.2",
"ejs": "^2.5.2",
"fs-extra": "^0.30.0",
"mkdirp": "^0.5.1",
"mocha": "^3.1.2",
Expand Down
Loading