Amber: [CLI] Redesign Inclusion of DB-related Shards

[docelic]

Currently, a new project which defaults to Pg still depends on all DB shards and they are downloaded when one runs 'shards', including the ones not needed/used:

redis
mysql
sqlite3

This should be sorted out and according to @marksiemers' advice, maybe these should be on app level rather than framework level.

[marksiemers]

This will require having conditional require statements in some places, not sure what else.

Example: https://github.com/amberframework/amber/blob/master/src/amber/cli/commands/database.cr#L4

[faustinoaq]

This will require having conditional require statements in some places, not sure what else

Yeah, a conditional require would work here, we should use macros though

{% if database == "pg" %}
require "pg"
{% elsif database == "mysql" %}
require "mysql"
{% elsif database == "sqlite" %}
require "sqlite3"
{% end %}

[eliasjpr]

Was this resolved?

[elorest]

@faustinoaq @marksiemers require is a before macro operation. You can't conditionally do requires with macros.

[marksiemers]

I don't think this should be closed. The macro solution may not work, but something could.

The require statements are done in the CLI, which does make it a challenge - maybe some kind of dependency injection could work, or somehow offloading the operations and dependencies to the app itself.

It may be a for a far future version, but we shouldn't drop it.

[eliasjpr]

👍🏻 I closed it thinking this was addressed in he above PR

[faustinoaq]

require is a before macro operation. You can't conditionally do requires with macros.

@elorest I tested some code, and I think we can try something like this:

VAR = "no"  # Try changing this to "yes"

{% if VAR == "yes" %}

require "tempfile"

pp Tempfile.new("foo").path

{% elsif VAR == "no" %}

require "colorize"

pp "foo".colorize(:green)

{% end %}

https://carc.in/#/r/3k9q

Or maybe we can use some kind of ECR template to generate db-related files, WDYT?

[marksiemers]

Based on some playing in carc.in a macro will change what is required: https://carc.in/#/r/3khf

[eliasjpr]

@marksiemers and @faustinoaq can you guys take a shot at addressing this in a PR? I think this can be an easy win :)

[marksiemers]

@eliasjpr - if the code with the require statements were in the app itself, this would be an easy win, but they are in the CLI. Since that is built independent of any application - that makes this more difficult.

Moving the dependency to each individual app, but having the CLI still work may require re-architecting this whole section of the CLI - not just putting in a couple macro statements.

[eliasjpr]

One way on how we can clean this dependency is by moving the CLI out of the project this way we don't need the DB shards to exist on the Amber repo.

See https://github.com/amberframework/amber-cli

[eliasjpr]

We are going to close this for now and revisit at a later time given that this might required to split the CLI to its own project.