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

Update CLI and API #50

Merged
merged 10 commits into from
May 12, 2017
Merged

Update CLI and API #50

merged 10 commits into from
May 12, 2017

Conversation

bclinkinbeard
Copy link
Contributor

Implements the changes discussed in #40.

Also includes a good amount of path code cleanup. I think it's a good idea to centralize and quarantine the code that deals with paths as much as possible. The CLI doesn't do anything with paths now, its sole responsibility is providing and documenting the CLI API. It doesn't do anything besides passing the options to the JS function.

This also brings all of the various ways to specify options into harmony by way of matching names. Options specified on the command line and/or passed in the options object to the function override options specified in package.json override default options. See the alias method in the bin file for a list of option names that is easy to read and review.

@bclinkinbeard
Copy link
Contributor Author

Added a commit to remove the package.json config from the override cascade since components means something different in that context.

@bclinkinbeard
Copy link
Contributor Author

Two questions have to be answered before we can land this.

  1. Default to file watching with a --build option or default to a single build with a --watch option?

  2. Minify by default with a --debug option or don't minify by default with a --prod option?

I vote for --watch and --prod as they are common across a lot of tools.

@mathisonian
Copy link
Member

Thanks @bclinkinbeard!

I'm happy to switch over to opting in to watch, for the JS api this seems clearly the right choice. For the CLI its less obvious, but I think its nice to have consistency, and is in line with how some other tools behave.

For minifying - I'd like to keep that opt-out for the build step. One thing that Idyll does now that I'd like to keep is to default to best-practices unless you explicitly opt out of them. I'd also vote to keep the option named as minify for clarity unless you forsee other things getting rolled into that in the near future.

Copy link
Member

@mathisonian mathisonian left a comment

Choose a reason for hiding this comment

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

Looks much cleaner! I'm going to do a bit of testing locally

src/index.js Outdated
const getPath = getPathFactory(inputDir);
const inputPkg = require(getPath('package.json'));
const inputCfg = (inputPkg.idyll || {components: {}});
for (let key in inputCfg.components) {
Copy link
Member

Choose a reason for hiding this comment

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

Small style thing - can we keep things like inputCfg spelled out like inputConfig, this makes the code more friendly and readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, will fix that

bin/idyll.js Outdated

idyll(path.resolve(argv._[0]), options);
Copy link
Member

Choose a reason for hiding this comment

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

I added this path.resolve in to resolve some weird bug when using idyll linked locally. Will test this to make sure everything still works.

bin/idyll.js Outdated
.describe('theme', 'Name of (or path to) the theme to use')
.default('theme', 'idyll')
.alias('h', 'help')
.argv;
Copy link
Member

Choose a reason for hiding this comment

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

👍 so much cleaner

@mathisonian
Copy link
Member

We'll need to update the project generator too to match this

@mathisonian
Copy link
Member

I'm getting some weird behavior locally -

This is running the script with idyll linked locally:

    "start": "idyll index.idl --css styles.css --layout centered --theme github --spellcheck"
  • It seems like the compiler is being triggered a bunch of times on startup (spellcheck keeps re-running)
  • The styles are 404'ing

idyll-api-updates

@bclinkinbeard
Copy link
Contributor Author

Hmm, I admittedly use npm run build most of the time when testing so it's possible I overlooked something. I will check into that now.

Regarding the CLI, you want to minify by default and turn it off with --minify false?

@bclinkinbeard
Copy link
Contributor Author

Or maybe --no-minify is better?

@mathisonian
Copy link
Member

👍 --no-minify

@bclinkinbeard
Copy link
Contributor Author

All issues addressed, except the npm start thing as that is coming from the chokidar stuff in master. I will address that in another PR.

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.

None yet

2 participants