Allow specifying environment variables in .ember-cli by astronomersiva · Pull Request #7910 · ember-cli/ember-cli

astronomersiva · 2018-07-11T15:58:17Z

Addresses #7896.

Makes it possible to specify environment variables in .ember-cli like

     {
          "environment": {
               "EMBER_CLI_MODULE_UNIFICATION": true
          }
     }

rwjblue · 2018-07-11T16:10:32Z

lib/experiments/index.js

+  primary: path.dirname(cliPath),
+});
+const experimentsInDotCli = emberCli.get('experiments') || {};
+Object.assign(process.env, experimentsInDotCli);


Hmm, I'd prefer to do this where we do the rest of the yam stuff (around here). I also think that if we are "just" merging with process.env we shouldn't use experiments as the key in .ember-cli (we would use environment or something else)...

That was my original intention as well but there was an issue with that.

We require models/project here which requires lib/experiments internally. Due to this, the experiments are set even before .ember-cli is loaded.

Placing the Project require below the getConfig also won't help as getConfig again uses lib/experiments internally.

I agree that the key name has to be changed to environment in .ember-cli.

astronomersiva · 2018-07-17T17:34:39Z

@rwjblue , I have made the suggested changes. Please take a look :)

twokul · 2018-07-17T21:15:43Z

might be a silly idea but why not teach yam to understand process.env as a fallback? that way all configuration is still handled in one place and we don't need to make changes to cli itself?

astronomersiva · 2018-07-18T17:18:57Z

@twokul, I am not very convinced about having process.env as a fallback for the following reasons:

I might have certain env variables in .ember-cli's environment property. It will not be possible to temporarily and quickly try out some other value for one of those env variables by just doing SOME_VAR=SOMETHING_ELSE ember s as .ember-cli will be the primary source.
env variables will be present in process.env and .ember-cli's environment field. We get the config from .ember-cli itself in get-config. We will have to make Yam use the environment field in .ember-cli as the primary source and process.env itself as the secondary source.

Apologies if I have misunderstood your comment :)

twokul · 2018-07-19T17:38:12Z

@astronomersiva

We will have to make Yam use the environment field in .ember-cli as the primary source and process.env itself as the secondary source.

is there something wrong with that? 😄

astronomersiva · 2018-07-20T04:28:38Z

@twokul , maybe I am not understanding you here properly but the whole point of this PR was to enable people to put their ENV variables in .ember-cli. Using Yam to have process.env as a secondary source for .ember-cli is not going to help with that.

is there something wrong with that?

Consider an env variable MY_ENV_VARIABLE present in both process.env and inside the environment field in .ember-cli. AFAIK, you can use Yam to read a file only. So, even if you have process.env as the secondary source, the config object provided by Yam will be like this:

let config = new Yam('ember-cli', {
    primary: project_root
    secondary: process.env // i believe this is what you are talking about
});

// config will be 
{
    MY_ENV_VARIABLE: 'some value'         // taken from process.env
    environment: {
        MY_ENV_VARIABLE: 'some_value'    // taken from .ember-cli
    }
}

We will now have to get the values present in the environment field, iterate over them and verify they are not present in process.env directly to ensure the behaviour I have written about in the first part of my previous comment.

One solution I see for this is to make Yam automatically treat values inside the environment field as special and assign those values to process.env after doing the same checks that I have done here.

astronomersiva · 2018-07-25T04:51:55Z

@rwjblue would really appreciate if you take another look at this.

rwjblue

I left some questions inline, and a couple small tweaks in the test but other than that this looks really good, thank you for working on it!

rwjblue · 2018-07-25T13:29:33Z

lib/cli/index.js

+    // * env variables passed directly should have more priority over .ember-cli.
+    // * process.env will have only string values. Therefore, check if the value
+    //   in .ember-cli is truthy and then assign to process.env.
+    if (!process.env.hasOwnProperty(key) && value) {


I think I would do if (!(key in process.env)) {}, also why does value need to be truthy? I'm concerned about null and 0 as values, where a normal process.env.something = null would coerce to 'null'...

That makes sense, thanks 👍

rwjblue · 2018-07-25T13:30:46Z

tests/unit/settings-file/env-variables-test.js

+      '.ember-cli': JSON.stringify({
+        environment: {
+          "MY_ENV_VARIABLE": "Yayy",
+          "SHOULDNT_BE_THERE": false,


Not sure about this one (see question above)

rwjblue · 2018-07-25T13:31:01Z

tests/unit/settings-file/env-variables-test.js

+    });
+  }));
+
+  after(co.wrap(function *() {


I think you want this to be afterEach

rwjblue · 2018-07-25T13:31:13Z

tests/unit/settings-file/env-variables-test.js

+describe('.ember-cli environment options', function() {
+  let sampleApp;
+
+  before(co.wrap(function *() {


Seems like it should be beforeEach

astronomersiva · 2018-07-25T15:17:04Z

@rwjblue , thanks for your time 😄 I have made the suggested changes.

knownasilya · 2018-07-26T15:36:12Z

👏

rwjblue · 2018-07-26T15:37:04Z

Thank you @astronomersiva!

kellyselden · 2018-07-26T19:19:15Z

Seems to work well here kellyselden/ember-cli-rollup-packager#30. Nice work!

astronomersiva · 2018-07-26T19:47:41Z

@kellyselden , thanks :) I also noticed that it is possible to have the .ember-cli file as a JS file. Might help if you want to do some checks within the file or maybe even get rid of all the double quotes for the keys.

gossi · 2018-07-27T10:04:48Z

Thank you so much for this ❤️

nightire · 2018-08-11T15:55:33Z

@astronomersiva there is a problem, if you import config/environment in your app, config.environment will be replaced as "environment" in .ember-cli, but it used to be process.env.EMBER_ENV. It is important sometimes to determine the environment variable in applications.

before this change:

after this change:

tested this feature in ember-cli v3.3

ef4 · 2018-08-16T18:07:32Z

@nightire You're right and that is very surprising to me. I filed #7980 for that and an additional similar case.

lougreenwood · 2018-08-20T15:15:57Z

What is the recommended way to access these environment vars in Ember? Looks like I could use the environment mentioned above, but I get the impression that is a bug and will likely be reverted?

ef4 · 2018-08-20T17:24:20Z

@lougreenwood your config/environment.js runs during the build process (in node, not the browser) so it has access to environment variables. That’s the typical way to get an env car into your app at runtime.

(That doesn’t have anything directly to do with this new feature for automatically setting env vars from the .ember-cli file.)

lougreenwood · 2018-08-20T17:27:39Z

I'm not sure I follow - are you saying that any values set in the new environment object will be merged into and made available in process.env in for usual consumption in config/environment.js?

ef4 · 2018-08-20T17:34:55Z

Yes. That (and only that) was the intended behavior of this feature.

lougreenwood · 2018-08-20T17:54:25Z

Nevermind, seems it was an obvious solution for my use case where process doesn't exist for the final compiled app (e.g in your wonderful Prember add-on 😉):

const EmberCliConfig = require('../.ember-cli');

If anyone's interested in my use case (or maybe Google is...) I'm trying to define config env vars for my app which has access to process.env for local dev (ember serve), but falls back to some string config value (injected by CI build), for deployment to a server with pre-rendered static HTML (no node for process.env).

This new feature which allows me to define them in .ember-cli.js allows me to:

move all definitions and string replacement targets into one file (was previously replacing in every pre-rendered *.html)
detect and set use fallback when in CI

My config definitions are now much cleaner and more robust as a result and also improves my Cordova build pipeline :)

rwjblue requested changes Jul 11, 2018

View reviewed changes

astronomersiva force-pushed the experiment_flags_in_ember_cli branch 4 times, most recently from c9c7f6d to c8665ed Compare July 17, 2018 17:31

astronomersiva changed the title ~~[WIP] Allow specifying experiments in .ember-cli~~ Allow specifying experiments in .ember-cli Jul 17, 2018

astronomersiva changed the title ~~Allow specifying experiments in .ember-cli~~ Allow specifying environment variables in .ember-cli Jul 18, 2018

twokul mentioned this pull request Jul 19, 2018

Meeting notes, July 19 emberjs/core-notes#253

Closed

rwjblue requested changes Jul 25, 2018

View reviewed changes

astronomersiva force-pushed the experiment_flags_in_ember_cli branch 2 times, most recently from 133953a to 8a7a329 Compare July 25, 2018 15:14

Allow specifying ENV variables in .ember-cli

d825561

astronomersiva force-pushed the experiment_flags_in_ember_cli branch from 8a7a329 to d825561 Compare July 25, 2018 15:15

rwjblue approved these changes Jul 25, 2018

View reviewed changes

rwjblue merged commit cac9a1a into ember-cli:master Jul 26, 2018

ef4 mentioned this pull request Aug 16, 2018

Breaking changes to meaning of environment #7980

Closed

lougreenwood mentioned this pull request Aug 20, 2018

Incorrect environment returned by EmberApp.env() in .ember-cli.js #7989

Closed

astronomersiva mentioned this pull request Sep 1, 2018

Revert "Allow specifying ENV variables in .ember-cli" #8008

Merged

NullVoxPopuli mentioned this pull request Sep 19, 2018

Environment variables in ember cli file #8059

Closed

Conversation

astronomersiva commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwjblue Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

astronomersiva Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astronomersiva commented Jul 17, 2018

Uh oh!

twokul commented Jul 17, 2018

Uh oh!

astronomersiva commented Jul 18, 2018

Uh oh!

twokul commented Jul 19, 2018

Uh oh!

astronomersiva commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astronomersiva commented Jul 25, 2018

Uh oh!

rwjblue left a comment

Choose a reason for hiding this comment

Uh oh!

rwjblue Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

astronomersiva Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

rwjblue Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

rwjblue Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

rwjblue Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

astronomersiva commented Jul 25, 2018

Uh oh!

knownasilya commented Jul 26, 2018

Uh oh!

rwjblue commented Jul 26, 2018

Uh oh!

kellyselden commented Jul 26, 2018

Uh oh!

astronomersiva commented Jul 26, 2018

Uh oh!

gossi commented Jul 27, 2018

Uh oh!

nightire commented Aug 11, 2018

Uh oh!

ef4 commented Aug 16, 2018

Uh oh!

lougreenwood commented Aug 20, 2018

Uh oh!

ef4 commented Aug 20, 2018

Uh oh!

lougreenwood commented Aug 20, 2018

Uh oh!

ef4 commented Aug 20, 2018

Uh oh!

lougreenwood commented Aug 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

astronomersiva commented Jul 11, 2018 •

edited

Loading

astronomersiva Jul 12, 2018 •

edited

Loading

astronomersiva commented Jul 20, 2018 •

edited

Loading