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

Environments #135

Merged
merged 2 commits into from
Sep 1, 2016
Merged

Environments #135

merged 2 commits into from
Sep 1, 2016

Conversation

cmolina
Copy link
Contributor

@cmolina cmolina commented Aug 23, 2016

implementation of #134

Carlos Molina added 2 commits August 23, 2016 10:49
For example, to make a build for android in the QA enviroment, you need to run:

    ENV=qa ionic build android
@codecov-io
Copy link

Current coverage is 91.95% (diff: 100%)

No coverage report found for master at 7fa4a20.

Powered by Codecov. Last update 7fa4a20...92155ae

@lathonez
Copy link
Owner

Just went to merge this.

What's the purpose of the config/<env>/app directories and the .keep files therein?

https://github.com/camolin3/clicker/tree/enviroments/config/dev/app

@cmolina
Copy link
Contributor Author

cmolina commented Aug 24, 2016

The idea is to place specific files for each environment.
For example, imagine exposing a service with API urls in config/dev/app/services/constants.ts

export class constants {
    public static get API_ENDPOINT(): string { return 'http://localhost:5432/api/'; }
}

You need to place the appropriate constants.ts file for each environment, inside config/<env>/app/services/. When compiling the app, the corresponding constants class will be copied into app/services/constants.ts.

GIT only track files but not empty directories, so thats why I added the .keep files in each config/<env>/app/ directory to keep its structure. Maybe a better idea is to add this example constants class.

@cmolina cmolina changed the title Enviroments Environments Aug 24, 2016
@lathonez
Copy link
Owner

Ah I see!

Maybe a better idea is to add this example constants class.

Yes, I don't think it's really clear what those folders are for, though from the gulp file you can infer it. It all feels quite implicit, but with an example it's clearer.

Duplicating the apps' directory structure inside the config directory (for whichever files need access to config), per environment, seems excessive to me. I'd much rather have one config.ts in app/config/<env> and namespace variables therein:

export config {
  services: {
    'API_ENDPOINT': 'http://localhost:5432/api/'
  }
}
import { config } from '../config';
console.log(config.services.API_ENDPOINT);

The above syntax is almost certainly incorrect but I hope it's instructive to what I'm getting at.

For one given component / service I may currently have four files:

  • file.ts
  • file.spec.ts
  • file.e2e.ts
  • file.mock.ts

I don't want to add another file for it in a different directory structure elsewhere.

However! This is all extremely subjective. If the app/config/<env>/app/... is working for you in various environments and you think it's beneficial over the above, I'm happy to pursue it here, though I'd appreciate a justification of the benefits of that approach.

Thanks!

@cmolina
Copy link
Contributor Author

cmolina commented Aug 26, 2016

Duplicating the apps' directory structure inside the config directory (for whichever files need access to config), per environment, seems excessive to me. I'd much rather have one config.ts in app/config/ and namespace variables therein

I agree with you. The problem with your approach is, that which of the app/config/<env>/constants.ts file should I load? I need to write

import { config } from '../config/dev/constants';
// or maybe
import { config } from '../config/qa/constants'
// or
import { config } from '../config/prod/constants'
// ...

Now we need to decide on runtime which file to use; I rather prefer to keep environment logic outside of the application code.

Maybe, instead of replicating all the filesystem, we could place the env-specific files just inside config/<env>/. We go from this

config/dev/app/services/constants.ts

to this

config/dev/constants.ts

and modify the gulp task to move all the .ts files inside app/. I should probably need only one file with constants.

@lathonez
Copy link
Owner

modify the gulp task to move all the .ts files inside app/. I should probably need only one file with constants.

Yes this is exactly what I meant, apologies if it was unclear:

import { config } from '../config/constants';

where the gulp task populates app/config/constants at build time with the relevant file from config/env.

@lathonez
Copy link
Owner

If I get to this before you I will make those changes and merge in. I think it would be instructive to have an example with a log line somewhere in the app that gets loaded from config/constants.

If you have time to make such an example please feel free! I'll put a message on this before I start so there's no duplicating effort.

Thanks again for the PR

@lathonez
Copy link
Owner

Not ignoring this, just very busy. Will be merged when I have time.

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