[configure-splash-screen] Refactor and integrate with `@expo/config` #2297

bbarthec · 2020-06-22T19:50:50Z

Why

@expo/config should autoconfigure the splash screen once expo eject is called instead of showing nasty warning.

How

This whole PR is much too big and complex to be called a nice one 😅
The amount of code that has been written is not even to be compared to the amount of functionality this PR provides (to the disadvantage of the the first).
Nevertheless I've decided to try to ship it in the shape it is.
Why? Because I've experimented with:

generic SplashScreenConfig type
platform-specific SplashScreenConfig types that are created dynamically via mapped type
JSON-friendly types equivalents that are to be exposed outside the package (e.g. for @expo/config) and are dynamically generated from original types via other mapped type
Validator object that has this nicely looking addRule method that is fully TS-supported
cli command separated completely from logic now (only defines CLI options and performs minimal validation and shaping raw JSON Config for further processing)
a bunch of tests (these might and should be cleaned in the future - I started to wonder how to write minimal set of tests to cover each code path 🤔 any idea here?)

packages/configure-splash-screen/src/types.ts

packages/config/src/android/SplashScreen.ts

packages/config/src/ios/SplashScreen.ts

packages/configure-splash-screen/src/android/Colors.xml.ts

brentvatne · 2020-07-15T03:56:42Z

packages/configure-splash-screen/src/validators/FromJsonValidator.ts

+/**
+ * This class is responsible for validating configuration object in a form of json and produce validated object based on validating `rules` added via `addRule` method.
+ */
+export default class FromJsonValidator<From extends JsonShape<To>, To extends object> {


this class is pretty clever. we should probably pull it out into a util. out of curiosity, did you consider any other npm packages to do a similar thing? i'm not familiar with any that both transform and validate like this does

Congratulations on implementing an object validator! I too am curious whether you considered other NPM packages. As I mentioned on one of the calls I've had pretty good experience with zod which, I guess, does the same thing (parses an unknown object into an object of a known TS type, inferring and parsing types and optionally validates)?

While I admire the heart you've put into this and stuff you must have learned along the way I'm a little afraid maintaining this may be a burden no one will want to take a couple of months from now and this will become a zombie file here be dragons.

Well, it was my little experiment 😉
I really like the zod idea and I'm willing to add it to our codebase somewhere. I'm not sure though if zod can provide validation & transformation at the same time, please take a look at following lines:
https://github.com/expo/expo-cli/pull/2297/files#diff-63c94c9e1d2966b124e8333ec934d795R55
https://github.com/expo/expo-cli/pull/2297/files#diff-63c94c9e1d2966b124e8333ec934d795R29
These represent the same think, but one in JSON and other in it's semantically-validated equivalent (Color is a tuple). What I'm curious about is: whether zod can convert for example string into some complex object/structure? 🤔 Nonetheless, IMO it's still worth to be added 👍

packages/configure-splash-screen/src/validators/__tests__/android.test.ts

packages/configure-splash-screen/src/validators/index.ts

brentvatne · 2020-07-15T04:07:37Z

I started to wonder how to write minimal set of tests to cover each code path 🤔 any idea here?

I think the current structure of this code lends itself very well to testing. If we test the validator/transformer functions exhaustively, then we can do some e2e tests that ensure they're wired up correctly with the CLI/external api.

One thing that we might want to do is split out the FromJsonValidator and test that independently, then just assume it works as it promises though its API within this code. We can do that in a followup PR.

packages/configure-splash-screen/src/__tests__/cli-command.test.ts

packages/configure-splash-screen/src/android/__tests__/Colors.xml.test.ts

packages/configure-splash-screen/src/cli-command.ts

bbarthec · 2020-08-27T18:14:07Z

Linking with #1494

sjchmiela · 2020-08-27T19:45:49Z

Oooh… thanks for reminding to review this 😅

sjchmiela

This is a very clever pull request (which means both good and bad). 😅 Had some questions, apart from them it looks very solid!

sjchmiela · 2020-08-27T20:34:18Z

packages/configure-splash-screen/src/validators/FromJsonValidator.ts

+/**
+ * This class is responsible for validating configuration object in a form of json and produce validated object based on validating `rules` added via `addRule` method.
+ */
+export default class FromJsonValidator<From extends JsonShape<To>, To extends object> {


Congratulations on implementing an object validator! I too am curious whether you considered other NPM packages. As I mentioned on one of the calls I've had pretty good experience with zod which, I guess, does the same thing (parses an unknown object into an object of a known TS type, inferring and parsing types and optionally validates)?

While I admire the heart you've put into this and stuff you must have learned along the way I'm a little afraid maintaining this may be a burden no one will want to take a couple of months from now and this will become a zombie file here be dragons.

packages/config/src/ios/SplashScreen.ts

sjchmiela · 2020-08-27T20:44:34Z

packages/configure-splash-screen/src/cli-command.ts

+const AVAILABLE_OPTIONS_NAMES = [
+  'imageResizeMode',
+  'platform',
+  'backgroundColor',
+  'imagePath',
+  'darkModeBackgroundColor',
+  'darkModeImagePath',
+  'statusBarHidden',
+  'statusBarStyle',
+  'darkModeStatusBarStyle',
+  'statusBarTranslucent',
+  'statusBarBackgroundColor',
+  'darkModeStatusBarBackgroundColor',
+] as const;
+
+type CLIOptionName = typeof AVAILABLE_OPTIONS_NAMES[number];


Why this sophisticated const AVAILABLE_OPTIONS_NAMES and then this typeof instead of a simple enum or a union type?

sjchmiela · 2020-08-27T20:45:38Z

packages/configure-splash-screen/src/cli-command.ts

+
+type CLIOptionName = typeof AVAILABLE_OPTIONS_NAMES[number];
+
+type CLIOptions = Partial<Record<CLIOptionName, string>>;


Wouldn't type of statusBarTranslucent be boolean?

sjchmiela · 2020-08-27T20:46:38Z

packages/configure-splash-screen/src/cli-command.ts

+  if (!backgroundColor) {
+    throw new Error(
+      `'backgroundColor': Required option is not provided. Provide a valid color string.`
+    );
+  }


I think recent releases of commander.js allow usage of requiredOption if that's something we may want to look into

Yeah, it does, but I couldn't get this option covered by tests then 😞 Commander.js simply kills the process with exit code 1 once the option is not provided. Maybe it does not need to be tested then? 🤔 Dunno 🤷

sjchmiela · 2020-08-27T20:49:08Z

packages/configure-splash-screen/src/cli-command.ts

+  }
+
+  const resolvedStatusBarHidden =
+    statusBarHidden === undefined ? undefined : Boolean(statusBarHidden);


Passing undefined in a variable called resolved seems icky to me. Is there a specific reason for passing undefined down? Does it say "do not modify anything related to this setting"?

Actually while writing this comment I had deja vu, so if we already talked about this then sorry

packages/configure-splash-screen/src/android/index.ts

…ig types - created generic SplashScreenConfig type - created platform-specific SplashScreenConfig types - created JSON-friendly equivalent of above-mentioned types - created mapping type for modyfing arbitrary type with new subschema - created mapping type for generatig JSON-friendly type equivalent from given type

…ith new bin and main entries

…stions

… in config types

EvanBacon · 2020-09-07T11:04:55Z

packages/configure-splash-screen/src/__tests__/cli-command.test.ts

@@ -0,0 +1,138 @@
+import configureAndroid from '../android';


We should rename all of these tests to -test after this PR is merged. #2591

EvanBacon · 2020-09-07T11:27:29Z

Seems the XML logic isn't unified with expo/config so running expo eject/apply formats the XML files differently.
This doesn't work as expected when it's rerun using apply. Apply should be supported because it makes using bare far better for expo users, but also having apply support makes changes like this substantially easier to test as a whole.

brentvatne · 2020-09-10T00:38:50Z

Seems the XML logic isn't unified with expo/config so running expo eject/apply formats the XML files differently.

@bbarthec @EvanBacon - let's chat at the next expo-cli meeting about which xml library we should be using, we currently have three (that i'm aware of? maybe more?):

xmldom
xml2js
xml-js

we need to consolidate on one to reduce duplicate dependencies and ensure that we're not reformatting the xml files in subtle ways when different commands are executed.

…e-with-config

bbarthec · 2020-09-14T15:58:22Z

Do you guys have any major concerns about this PR apart from the xml library? cc @brentvatne, @EvanBacon
If no, wy not to ship it now and work on the top of it in the separate PRs?

…e-with-config

…ming convention (#2749) As mentioned in issue #2591 and PR #2297, we want to update tests from `.test` to `-test`.

bbarthec marked this pull request as ready for review June 22, 2020 20:22

bbarthec requested review from brentvatne, tsapeta, sjchmiela and mczernek June 22, 2020 20:22

brentvatne requested a review from satya164 July 8, 2020 20:30

brentvatne reviewed Jul 15, 2020

View reviewed changes

packages/configure-splash-screen/src/types.ts Outdated Show resolved Hide resolved

brentvatne reviewed Jul 15, 2020

View reviewed changes

satya164 reviewed Jul 15, 2020

View reviewed changes

sjchmiela reviewed Aug 27, 2020

View reviewed changes

bbarthec added 11 commits September 4, 2020 16:16

[configure-splash-screen] feat: move utils from ./helpers to ./utils

86fddc0

[configure-splash-screen] fix: make one pbxproj propoerty opional

0172fa0

[configure-splash-screen] feat: clean-up js constants

7ba4a37

[configure-splash-screen] feat: create Validator for ConfigJson

75f35f1

[configure-splash-screen] feat: create external API

ba09414

[configure-splash-screen] feat: update CLI command and package.json w…

aa090b5

…ith new bin and main entries

[configure-splash-screen] feat: update and write missing tests

bd8474e

[config] feat: add configure-splash-screen and remove warning

3e15875

[config] fix: rebase with master

4ba291f

[configure-splash-screen] Simplify config validator & adjust PR sugge…

755420d

…stions

bbarthec force-pushed the @bbarthec/configure-splash-screen-integrate-with-config branch from 41680ca to 755420d Compare September 4, 2020 16:56

bbarthec added 2 commits September 4, 2020 19:09

[configure-splash-screen] Rename imagePath into image in internally &…

8481c9c

… in config types

[configure-splash-screen] Remove TODO comment, because it's already done

cc77d1c

bbarthec requested review from sjchmiela and EvanBacon September 4, 2020 17:23

EvanBacon mentioned this pull request Sep 7, 2020

Rename all of tests from .test to -test #2591

Closed

EvanBacon reviewed Sep 7, 2020

View reviewed changes

EvanBacon added 2 commits September 10, 2020 22:19

Merge branch 'master' into @bbarthec/configure-splash-screen-integrat…

560d054

…e-with-config

Merge branch 'master' into @bbarthec/configure-splash-screen-integrat…

8abf170

…e-with-config

bbarthec requested review from brentvatne and EvanBacon September 14, 2020 15:56

EvanBacon and others added 3 commits September 16, 2020 20:26

Merge branch 'master' into @bbarthec/configure-splash-screen-integrat…

d8f539a

…e-with-config

[splash-screen] Fix config not being applied

2b63608

Merge branch 'master' into @bbarthec/configure-splash-screen-integrat…

c94dcb5

…e-with-config

brentvatne approved these changes Sep 18, 2020

View reviewed changes

brentvatne merged commit 885f280 into master Sep 18, 2020

brentvatne deleted the @bbarthec/configure-splash-screen-integrate-with-config branch September 18, 2020 01:58

bbarthec mentioned this pull request Sep 18, 2020

[configure-splash-screen] <docs>: update usage description #2668

Merged

3 tasks

chernandez7 mentioned this pull request Oct 2, 2020

[configure-splash-screen]<chore>: Update test file names to follow naming convention #2749

Merged

2 tasks

bbarthec pushed a commit that referenced this pull request Oct 13, 2020

[configure-splash-screen]<chore>: Update test file names to follow na…

2e69621

…ming convention (#2749) As mentioned in issue #2591 and PR #2297, we want to update tests from `.test` to `-test`.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[configure-splash-screen] Refactor and integrate with `@expo/config` #2297

[configure-splash-screen] Refactor and integrate with `@expo/config` #2297

bbarthec commented Jun 22, 2020 •

edited

Loading

brentvatne Jul 15, 2020

sjchmiela Aug 27, 2020

bbarthec Sep 4, 2020

brentvatne commented Jul 15, 2020

bbarthec commented Aug 27, 2020

sjchmiela commented Aug 27, 2020

sjchmiela left a comment

sjchmiela Aug 27, 2020

sjchmiela Aug 27, 2020

sjchmiela Aug 27, 2020

sjchmiela Aug 27, 2020

bbarthec Sep 4, 2020

sjchmiela Aug 27, 2020

EvanBacon Sep 7, 2020

EvanBacon commented Sep 7, 2020

brentvatne commented Sep 10, 2020

bbarthec commented Sep 14, 2020


		type CLIOptionName = typeof AVAILABLE_OPTIONS_NAMES[number];

		type CLIOptions = Partial<Record<CLIOptionName, string>>;

[configure-splash-screen] Refactor and integrate with @expo/config #2297

[configure-splash-screen] Refactor and integrate with @expo/config #2297

Conversation

bbarthec commented Jun 22, 2020 • edited Loading

Why

How

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentvatne commented Jul 15, 2020

bbarthec commented Aug 27, 2020

sjchmiela commented Aug 27, 2020

sjchmiela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvanBacon commented Sep 7, 2020

brentvatne commented Sep 10, 2020

bbarthec commented Sep 14, 2020

[configure-splash-screen] Refactor and integrate with `@expo/config` #2297

[configure-splash-screen] Refactor and integrate with `@expo/config` #2297

bbarthec commented Jun 22, 2020 •

edited

Loading