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

feat(stateConfig): make state config more flexible #38

Merged
merged 5 commits into from
Feb 26, 2020

Conversation

JamesSingleton
Copy link
Contributor

This allows the provideStateConfig to accept more than an object
or a string.

Closes #20

This allows the provideStateConfig to accept more than an object
or a string.

Closes #20
@JamesSingleton JamesSingleton requested a review from a team as a code owner February 13, 2020 23:50
@oneamexbot
Copy link
Contributor

oneamexbot commented Feb 13, 2020

☑️ Preflight Checklist:

All questions must be addressed before this PR can be merged.

  • What is the communication plan for this change?
  • Does any documentation need to be updated or added to account for this change? If so was it done already?
  • What is the motivation for this change?
  • Should these changes also be applied to a maintenance branch or any other long lived branch?
  • How was this change tested?
  • Does this change require cross browser checks? Why or why not?
  • Does this change require a performance test prior to merging? Why or why not?
  • Could this be considered a breaking change? Why or why not?
  • Does the change impact caching?
  • Does the change impact HTTP headers?
  • Does the change have any new infrastructure requirements?
  • Does the change affect other versions of the app?
  • Does the change require additional environment variables?
  • What is the impact to developers using the app?
  • What is the change to the size of assets?
  • Should integration tests be added to protect against future regressions on this change?

Generated by 🚫 dangerJS against d8853e7

@JamesSingleton
Copy link
Contributor Author

  • What is the communication plan for this change?
    • API Docs
  • Does any documentation need to be updated or added to account for this change? If so was it done already?
  • What is the motivation for this change?
  • Should these changes also be applied to a maintenance branch or any other long lived branch?
    • No
  • How was this change tested?
    • Unit tests
    • Manually serving a root module to one-app
  • Does this change require cross browser checks? Why or why not?
    • N/A
  • Does this change require a performance test prior to merging? Why or why not?
    • N/A
  • Could this be considered a breaking change? Why or why not?
    • No, relaxing the check on provideStateConfig
  • Does the change impact caching?
    • No
  • Does the change impact HTTP headers?
    • No
  • Does the change have any new infrastructure requirements?
    • No
  • Does the change affect other versions of the app?
    • No
  • Does the change require additional environment variables?
    • No
  • What is the impact to developers using the app?
    • Allows developers to provide a more flexible provideStateConfig
  • What is the change to the size of assets?
    • N/A
  • Should integration tests be added to protect against future regressions on this change?

Copy link
Contributor

@mtomcal mtomcal left a comment

Choose a reason for hiding this comment

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

Looks great to me. We should add a test case to Integration test to see if non string is parsed correctly as well.

Copy link
Member

@10xLaCroixDrinker 10xLaCroixDrinker left a comment

Choose a reason for hiding this comment

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

Yes it should be updated in this PR

docs/api/modules/App-Configuration.md Outdated Show resolved Hide resolved
@@ -123,8 +123,7 @@ export const setStateConfig = (providedStateConfig) => {
throw makeConfigEnvError();
}
acc.client[key] = client[configEnv];
}
if (typeof client === 'string') {
} else {
Copy link
Member

Choose a reason for hiding this comment

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

keep validating the type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we want to only explicitly accept string, number, or boolean? The reason I did this was it puts the complexity to 13. I talked with @JAdshead and we came up with this solution.

@JamesSingleton
Copy link
Contributor Author

Looks great to me. We should add a test case to Integration test to see if non string is parsed correctly as well.

@mtomcal,

This would require a 0.0.2 of PickyFrank to be created since the current way we log things, it throws at the first error instead of batching everything together.

@JamesSingleton JamesSingleton merged commit 72539a0 into master Feb 26, 2020
@JamesSingleton JamesSingleton deleted the feat/flexible-config branch February 26, 2020 23:15
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.

The config from the root module should be more flexible
5 participants