Make manifest.env available in publish and export commands

[rodrigorm]

Make process.env variables available in manifest.env in expo publish and expo export commands like in expo start allowing a project to have a app.json with settings which can be set using environment variables, eg:

In app.json:

{
  "expo": {
    "extra": {
      "apiEndpoint": "http://www.example.com/api"
    }
  }
}

In src/App.js:

import { Constants } from 'expo';

class App extends Component {
  constructor(props) {
    super(props);
    this.apiUrl = process.env.EXPO_API_URL || Constants.manifest.extra.apiUrl;
  }
}

$ # Following already works in `start` command
$ EXPO_API_URL="http://localhost:8080/api" expo start
$ # Usage in `publish` command
$ EXPO_API_URL="http://localhost:8080/api" expo publish

[brentvatne]

hey there! sorry nobody got back to you on this.

i think i would suggest using babel-plugin-inline-environment-variables instead of building similar functionality into expo-cli. open to be convinced, but i'll close this pr for now until i am convinced :)

[rodrigorm]

Hi Brent, I appreciate the attention, In my defens `expo start` already has this functionality available, I am just trying to make the same for other commands. This make the expo more `Twelve-Factor`ish and enable publishing different configuration to separate release channels more easily. Em sex, 6 de dez de 2019 às 20:18, Brent Vatne <[email protected]> escreveu:

…

hey there! sorry nobody got back to you on this. i think i would suggest using babel-plugin-inline-environment-variables <https://babeljs.io/docs/en/babel-plugin-transform-inline-environment-variables> instead of building similar functionality into expo-cli. open to be convinced, but i'll close this pr for now until i am convinced :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#540?email_source=notifications&email_token=AAAFUHR5DSKTCI5E4FANBD3QXLMURA5CNFSM4HH5V2D2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGFU6JI#issuecomment-562777893>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFUHQONCTYFFFJVZAEU6TQXLMURANCNFSM4HH5V2DQ> .

[brentvatne]

i'm not crazy about this existing on expo start but you raise a good point that it would seem to be consistent to have expo publish behave the same. i'll have to investigate the justification for including it on expo start in the first place to decide what we should do here. i appreciate your clarification, thanks!

[brentvatne]

ok i think this is reasonable to include. really sorry for the delay and back and forth on it. if you can resolve conflicts then i'll happily merge it. thanks @rodrigorm!

[rodrigorm]

@brentvatne resolved the conflicts, but I am not sure if it is all working because the packages structure changed since my PR is open. Can you help me with the tests to verify the expected changes is working?

[brentvatne]

hey @rodrigorm - thank you! I'll get back to you on this after the holiday

[brentvatne]

the change that you made should work fine, but i've thought about this more and i don't think that this is a safe change for us to make without some additional work. it's quite possible that some folks have environment variables with EXPO_ or REACT_NATIVE_ prefixes that contain sensitive information, and that others will do this in the future. this change would leak that information into the public via the published manifest.

i think an allow list (aka whitelist) rather than block list (aka blacklist) in app.json might be a safer way to expose environment variables. alternatively, we could put this behind an explicit flag or app.json config option

[brentvatne]

actually a cleaner way to do this will be possible with #1342

you could explicitly pull out env vars and pass them through the extra field

[rodrigorm]

@brentvatne you are right, variables like EXPO_APPLE_PASSWORD from other expo cli commands will be included in the manifest. Maybe it is better to remove this functionality even from the expo start command.

[brentvatne]

closing this for now, will follow up in #1435 when #1342 is ready :) thank you for the insights @rodrigorm!