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

Add a programmatic API (fixes #86) #113

Merged
merged 6 commits into from
Feb 19, 2017
Merged

Add a programmatic API (fixes #86) #113

merged 6 commits into from
Feb 19, 2017

Conversation

dar5hak
Copy link
Contributor

@dar5hak dar5hak commented Feb 16, 2017

This is a prototype of api.js as discussed in #86. Here's what I think it should do:

  • Export a function that takes options and directory as arguments
  • Start a server based on those
  • Print the server info nicely, with colours and stuff (tested on Linux)
  • Take stdin, such as ^C and behave accordingly
  • Return something to the caller (what?)
  • Write docs

@dar5hak
Copy link
Contributor Author

dar5hak commented Feb 16, 2017

Do you think this list is comprehensive? I'm still trying to figure out 4. Any hints on 5?

@leo
Copy link
Contributor

leo commented Feb 16, 2017

@dar5hak Please return an object with a .stop() method. This will solve both 4 and 5 as it allows people to stop the server programmatically. 🙏

lib/api.js Outdated
const path = require('path')

// { port: 1337 } => ['--port', '1337']
const optionsToArgs = options => {

Choose a reason for hiding this comment

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

can use dargs directly

@dar5hak
Copy link
Contributor Author

dar5hak commented Feb 17, 2017

I think it covers everything now.

@leo Could you have a final look?

@dar5hak
Copy link
Contributor Author

dar5hak commented Feb 17, 2017

I'll push the docs tomorrow. Added that to the checklist.

lib/api.js Outdated
module.exports = (options = {}, directory = process.cwd()) => {
const scriptPath = path.join(__dirname, '..', 'bin', 'serve.js')
const aliases = {cors: 'o'}
options._ = [directory] // Let dargs handle the directory argument
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add an empty line before this line.

lib/api.js Outdated
const aliases = {cors: 'o'}
options._ = [directory] // Let dargs handle the directory argument

const cli = spawn('node',
Copy link
Contributor

Choose a reason for hiding this comment

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

Please split it into multiple lines:

const args = [
  scriptPath,
  ...dargs(options, {aliases})
]

const cli = spawn('node', args, {
  stdio: 'inherit'
})

package.json Outdated
@@ -53,6 +54,7 @@
"boxen": "1.0.0",
"chalk": "1.1.3",
"copy-paste": "1.3.0",
"dargs": "^5.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please pin this dependency! => 5.1.0

@dar5hak
Copy link
Contributor Author

dar5hak commented Feb 18, 2017

Added docs. Please have a look at readme.md.

@dar5hak
Copy link
Contributor Author

dar5hak commented Feb 18, 2017

Also, a small problem. I couldn't get the ignore argument to work. How do you invoke it on the CLI?

serve --ignore readme.md package.json ./

This fails with the statement: Specified directory doesn't exist!

@leo
Copy link
Contributor

leo commented Feb 19, 2017

@dar5hak

serve --ignore "readme.md,package.json"

(here). I think you need to proxy the option, accept an array and then .join() the files together and pass the string to the command.

@tunnckoCore
Copy link

tunnckoCore commented Feb 19, 2017

@leo @dar5hak why use comma-separated values? Why not just accept a glob patterns and pass them to some fast matcher like nanomatch or micromatch. Nanomatch has strict support for only globs, without exglobs and etc. And it is Bash spec compliant.

Users are familiar with globs and matching, and think that nanomatch will fit great here, since it is small and support just globbing, so not so much needless features.

@leo
Copy link
Contributor

leo commented Feb 19, 2017

@tunnckoCore

  1. Not accepting comma-separated values and accepting glob values are two completely different things and aren't mutually exclusive.

  2. If we're going to support glob values, you'll still need to support normal strings as well. Otherwise it won't be possible to ignore a single file.

  3. If we're going to support glob values, we'll still use comma-separated values for that. Otherwise minimist will treat one glob value as the directory to serve. This means that we can just go on with this PR like I said here.

@dar5hak
Copy link
Contributor Author

dar5hak commented Feb 19, 2017

@leo There. Everything works now. 🎉

@leo leo merged commit 3aab1b2 into vercel:master Feb 19, 2017
@leo
Copy link
Contributor

leo commented Feb 19, 2017

@dar5hak Thanks a lot! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants