Skip to content
This repository was archived by the owner on Jan 20, 2019. It is now read-only.

Environment Selection #109

Closed
wants to merge 2 commits into from
Closed

Environment Selection #109

wants to merge 2 commits into from

Conversation

Copy link
Contributor

@hjdivad hjdivad left a comment

Choose a reason for hiding this comment

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

I like this approach

Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

Overall a big 👍 on this. I know determining environment with addons has always been a pain point, so this would be fantastic.

2. ember-cli specific process environment variable specified environment (e.g. `EMBER_ENV=production ember serve`)
3. General purpose process environment variable specified environment (e.g. `NODE_ENV=production ember serve`)
4. No environment was specified, default based on command type:
* Test commands -- Default to `test` environment.
Copy link
Member

Choose a reason for hiding this comment

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

Based on the definitions above, "test commands" could also be considered "interactive" (ember test --server) or "non-interactive" (ember test), therefore I think we should specify an order of precedence here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, thats what I was trying to do by ordering these bullet points. Specifically, this is why Test commands is first. I can redo it as an ordered list instead which would look like:

  1. Command line specified environment (e.g. ember serve --environment production)
  2. ember-cli specific process environment variable specified environment (e.g. EMBER_ENV=production ember serve)
  3. General purpose process environment variable specified environment (e.g. NODE_ENV=production ember serve)
  4. No environment was specified, default based on command type:
    1. Test commands -- Default to test environment.
    2. Interactive commands -- Default to development environment.
    3. Non-interactive commands -- Default to production environment.

Does that make it better?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I think just making it ordered should suffice.

}
```

This detection will be done early in the command process, and the `process.env.EMBER_ENV` environment variable will be set with the selected environment (so that addons and other tools are able to know the environment being used).
Copy link
Member

Choose a reason for hiding this comment

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

This may not be an issue, but having EMBER_ENV be the source of truth for which environment is being used seems potentially confusing when EMBER_ENV is not the highest specificity. Theoretically it would be possible to specify EMBER_ENV and a CLI flag in the same command and overwriting EMBER_ENV with the other value might cause confusion.

Again, may not be an issue, but something to consider.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it's something to consider, but I think there's a lot of value in

  1. Dealing with the various sources within ember-cli and giving addons a single place to look for the definitive environment
  2. For that single definitive place to be something standard like a process ENV var.

Copy link
Member Author

Choose a reason for hiding this comment

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

I totally agree that this is confusing, but changing this would be somewhat breaking (since we already do this here).

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @hjdivad, my concern is simply over potentially mutating a user-specified environment variable, but if it is already happening, then it seems likely to be a non-issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@trentmwillis i agree with the general concern around mutating user-specified env vars, but the only item that has precedence over EMBER_ENV in the rfc is the command line flag which means if the user specifies EMBER_ENV it will not be overwritten unless they also specify a command-line flag like --environment production

If a user does something like EMBER_ENV=development ember build -prod I don't think they'll think it too unreasonable for EMBER_ENV to change.

Copy link
Contributor

Choose a reason for hiding this comment

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

(also waiting for either you or rob to break the combo of everybody starting their replies by pointing out that they agree)

@rwjblue
Copy link
Member Author

rwjblue commented Jul 27, 2017

We had a discussion about this in the ember-cli core team call today. The team is generally 👍 to the long term goal, but would like a bit more massaging around implementing in a SemVer compatible way.

Specifically, the suggestion is:

  • Add a new flag to .ember-cli file (defaultNonInteractiveCommandEnvironment) which defaults to production for newly generated apps (and older apps that follow the standard ember init process).
  • Detect if the flag is not present, and issue a warning to the console but fallback to existing behaviors (e.g. development by default).
  • In a future version (possibly as early as 2.18.0 or as late as 3.0.0) remove the requirement for the flag, and use the scheme documented currently.

I'll work to update the RFC for that soon...

@rtablada
Copy link

@rwjblue putting more information in the motivations will make this RFC easier for people to understand, read, and give feedback.


We have received feedback from our direct users, developer relations folks, and benchmark maintainers suggesting that our current system for selecting the environment does not set our users up for success.

This is relevant for people close to core but outside of the news cycle and for those who aren't in the know on these events, this gives little to no context.
What feedback have we received?
What defaults do they recommend should be in place?
Why does having this new default mater?
More explanation of what this means would be helpful and there is not any mention of the current environment default, this is left to the user.

It would be helpful to give a little bit of background to the current environments and how they are used. Maybe something to the effect of:

Ember CLI currently allows users to specify the environment for any command using the flag `--environment`. This flag can be used by addons, build tooling, and the `config/environment.js` file to customize configuration, minification, and more.

I have a suggested alternative that we adjust and add things to the npm scripts:

The build script become ember build -e production.
Adding -e test to the test even though it is redundant.

From experience in other communities, they use the set of NPM scripts to teach people how to customize their own builds and show some common best practices.

The good thing about this alternative is that it would require no extra tooling and would have no breaking changes.
It also doesn't prevent the changes proposed in this RFC for future iterations.

@rwjblue
Copy link
Member Author

rwjblue commented Jan 19, 2019

Closed as part of the efforts to consolidate RFC repos, will reopen in emberjs/rfcs...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants