Skip to content
This repository has been archived by the owner on Jan 18, 2024. It is now read-only.

[config] Create dynamic config "app.config.js" #1342

Merged
merged 32 commits into from
Feb 5, 2020

Conversation

EvanBacon
Copy link
Contributor

@EvanBacon EvanBacon commented Dec 10, 2019

Why

Problems with the existing solution: app.json

  • can't swap between modes like development and production
  • no way to comment and document your config
  • can't share parts of your config across multiple projects
  • hard to discover what features are available
  • hard to know what everything in the config does
  • config types aren't in the monorepo and can't be easily changed

How

If a project has an app.config.js, that will take priority over an app.json. You can use the app.config.js to modify your app.json using environment variables or custom JS plugins. Here is how you might modify your project's app.json using the new app.config.js:

// config is generated from the package.json and the app.json.
module.exports = function({ projectRoot, config, mode }) {
  // Use the mode to control how the app works in dev vs prod
  config.name = mode === 'development' ? 'dev app' : 'prod app';
  // Return the modified config
  return config;
}

Also add support for the following config types:

  • app.config.js

  • app.config.json

  • app.config.json5 - disabled via @ide request

  • Added but disabled:

    • app.config.ts
    • app.config.yml
    • app.config.yaml
    • app.config.toml
  • I'm somewhat against the idea of having multiple different languages for the config. At the very least we could just add them later after app.config.js has migrated.

  • Only allow sync configs to maintain usability with Next.js.

Test Plan

  • Add tests for reading from and validating each language
  • Add a test for ensuring that app.config has higher priority than app.json
  • Add a test to ensure the app.config.js method is passed the app.json and a mode option
  • Ensure that other language app files don't get read (like app.yml, app.json5)
  • Enforce that the config is always serialized to JSON so it can be used for the manifest.

@EvanBacon EvanBacon requested a review from brentvatne December 10, 2019 19:28
@EvanBacon EvanBacon self-assigned this Dec 10, 2019
@brentvatne
Copy link
Member

one tradeoff worth noting here is that we won't be able to automate updating config for people after this, at least not with the .js extension.

so when we run expo upgrade we can't change the sdk version, and automating updating things like version/buildNumber etc will be more difficult in the future and for packages like @byCedric's semantic-release-expo.

re: sdk version, honestly we should move the source of truth for this to package.json anyways, and just use whatever sdk version corresponds to the expo package. that might be something we want to do before landing this.

are there any other tradeoffs you can think of?

@EvanBacon
Copy link
Contributor Author

Currently we do read the sdkVersion from the expo/package.json. sdkVersion is only used for locking down a version or using UNVERSIONED. Having a js config opens up room for plugins so we could potentially migrate scripts to dynamic classes that you add in the config.

@brentvatne
Copy link
Member

oh i did not realize we had made that change, nice!

@EvanBacon EvanBacon added the Expo config Related to the app.json config for Expo projects label Dec 12, 2019
@byCedric
Copy link
Member

byCedric commented Dec 12, 2019

Don't worry about the semantic release, it wasn't using Expo components anyways. I'll rework that library. If other automation tools are using the proper packages, like @expo/config, I don't think anyone else will have a lot of issues here.

Also, the getExpoSDKVersion is 🔥

@EvanBacon EvanBacon marked this pull request as ready for review December 13, 2019 10:33
@brentvatne
Copy link
Member

this is pretty big so i don't think i'll deploy it until the new year. will review it then as well :)

@brentvatne brentvatne added the needs review Issue is ready to be reviewed by a maintainer label Dec 20, 2019
@EvanBacon
Copy link
Contributor Author

Maybe we should also add expo icon to VSCode for app.config.js https://github.com/vscode-icons/vscode-icons/wiki/ListOfFiles

@EvanBacon
Copy link
Contributor Author

Added a method to ensure the config is always serialized before being read.

packages/config/src/getConfig.ts Outdated Show resolved Hide resolved
packages/config/src/getConfig.ts Outdated Show resolved Hide resolved
packages/config/src/getConfig.ts Show resolved Hide resolved
packages/config/src/getConfig.ts Outdated Show resolved Hide resolved
packages/config/src/Config.ts Show resolved Hide resolved
packages/config/src/Config.ts Outdated Show resolved Hide resolved
packages/config/src/Config.ts Outdated Show resolved Hide resolved
packages/config/src/Config.ts Outdated Show resolved Hide resolved
packages/config/src/Config.ts Outdated Show resolved Hide resolved
@fson fson self-requested a review January 22, 2020 14:46
Copy link
Contributor

@fson fson left a comment

Choose a reason for hiding this comment

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

This is very good I think.

I made a few comments regarding the developer experience – let's make sure the new format is easily debuggable and errors very clear from the beginning.

packages/config/src/getConfig.ts Show resolved Hide resolved

const configPath = options.configPath || customConfigPaths[projectRoot];

// If the app.json exists, we'll read it and pass it to the app.config.js for further modification
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we want to get rid of this capability, and make the JSON/JS configurations mutually exclusive, for a few reasons:

  • Having the configuration split across two files is more confusing for new users or anyone coming across a project that does this. Now you must know two ways to do the same thing. Now you need to check two files to confirm what configuration value is used.
  • When responding to issues the common question "please show us your app.config.js" becomes "please show us your app.config.js and app.json (and package.json)"
  • JS can include JSON inside it. It can also import configuration from an arbitrary JSON file, if a static format is desired for some values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having the configuration split across two files

We support this functionality for values that are generated by Expo CLI like bundleIdentifier. I'm totally open to better ideas though. I imagine a new project's config would look something like:

module.exports = function({ config }) {
  return {
  ...config,
  icon: 'assets/icon.png',
  /* etc... */
  }
}

The spread of another config is somewhat unpleasant.

responding to issues

We can add a mechanism for printing out the serialized config after it's been returned from app.config.js.

if a static format is desired for some values.

I imagine eventually app.json will be the static location for generated values.

Copy link
Member

Choose a reason for hiding this comment

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

When responding to issues the common question "please show us your app.config.js" becomes "please show us your app.config.js and app.json (and package.json)"

maybe we could add this to expo diagnostics? and we could automatically filter out any potentially sensitive fields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all we'd have to do to support this is switch the method used to read the config file.

packages/config/src/Config.ts Show resolved Hide resolved
packages/config/src/getConfig.ts Outdated Show resolved Hide resolved
@EvanBacon EvanBacon requested a review from fson January 30, 2020 21:53
@EvanBacon EvanBacon merged commit 88cf3a9 into master Feb 5, 2020
@EvanBacon EvanBacon deleted the @evanbacon/config/basic-app.config.js branch February 5, 2020 08:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Expo config Related to the app.json config for Expo projects needs review Issue is ready to be reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants