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 API-only CLI option #903

Closed
wants to merge 1 commit into from
Closed

Add API-only CLI option #903

wants to merge 1 commit into from

Conversation

sdzyba
Copy link

@sdzyba sdzyba commented Jun 28, 2018

Description of the Change

Now it is possible to tell amber watch to not to run npm process using api_only option.
api_only is a term borrowed from Rails' configuration. It might be reused in future for other API-specific functionality.

Related Amber recipe: amberframework/recipes#33

Alternate Designs

Something like no_npm option in .amber.yml might be used as well, although I find api_only more common and extendible in the future.
Also --no-npm argument for amber watch could be used but I find it unintuitive to use and this way we have some cli options in .amber.yml and some as arguments and this is inconsistent.

Benefits

User is able to run amber watch without npm dependency.

Possible Drawbacks

+1 another cli option to maintain.

@elorest
Copy link
Member

elorest commented Jun 28, 2018

I do like the no_npm option. However, I don't see the point of an api_only option. This has been discussed multiple times. The reason that Rails added an api only option was because they only have one middleware stack. So an api still has to run through all the same handlers as a web app which pointlessly slows stuff down.

With amber just create an api pipeline and use that. It won't be any slower if you have a web pipeline as well since the handler stack is completely different.

Rails and Amber have different paradigms when it comes to middleware.

@sdzyba
Copy link
Author

sdzyba commented Jun 29, 2018

Sounds reasonable.
If other people are ok with no_npm (or maybe the opposite: introduce npm option which is enabled by default?) I can change it this way.

@drujensen
Copy link
Member

@sdzyba I believe the new amber watch config that @faustinoaq is working on #865 will handle this option. You will be able to remove or replace the npm process. You will also be able to add other watch processes.

@eliasjpr
Copy link
Contributor

eliasjpr commented Jul 5, 2018

@sdzyba I agree with @drujensen, please review that PR instead #865 your review would be valuable

@eliasjpr
Copy link
Contributor

eliasjpr commented Jul 5, 2018

This should be closed in favor of #865

@sdzyba
Copy link
Author

sdzyba commented Jul 5, 2018

Got it.

@sdzyba sdzyba closed this Jul 5, 2018
@sdzyba sdzyba deleted the cli-api-only branch July 5, 2018 22:30
@faustinoaq
Copy link
Contributor

@sdzyba Yeah, thank your for your understanding, I have been a bit busy latest weeks I'll continue my work on #865 this weekend 😅

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.

5 participants