refactor: type global configuration file #7227
Draft
+509
−436
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Follow-up to a comment thread in #7199. This changeset adds types for the global configuration file (`$CONFIG_HOME/netlify/config.json`). This is mainly a step forward in documenting the configuration format. The type safety on it isn't ideal; e.g. you can try to read or write properties that don't exist in the config schema. That said, it's an improvement on the read side in the sense that any property you read is typed--even unknown ones default to `undefined`. This comes with two _potential_ breaking changes; I didn't find that they actually broke anything in practice, but they're worth looking out for in review: 1. We now set defaults for some values we did not previously set defaults for, e.g. `users[id: string].auth` defaults to an object. This could only break if a code path assumes e.g. the presence of `auth.github` means the user is authenticated; however, the way the types are structured marks all properties on that object as undefined, so this is a difficult error to make now. 2. We now validate that `users[id: string].id` is set. Failing to set it would make looking up user configuration impossible (we index into this object using the top-level `userId` field), so this prevents a class of mysterious logic errors. Stronger typing caused some issues in tests that mock the global config store; broke the `GlobalConfigStore`'s storage logic out into an adapter and implemented an in-memory adapter for use in tests. There are better ways to solve this problem--tests should be able to mock the global config without resorting to module mocking--but this will do for now. I think the global config store interface could use some work: at this point I think the dot-prop style of setting and getting properties only adds complexity vs something like a Proxy-wrapped object. That said, this is a step forward without committing to a bigger change.
ndhoule
force-pushed
the
refactor/type-global-config-file
branch
from
aedc633 to
cb611a9
Compare
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to a comment thread in #7199.
This changeset adds types for the global configuration file (
$CONFIG_HOME/netlify/config.json).This is mainly a step forward in documenting the configuration format. The type safety on it isn't ideal; e.g. you can try to read or write properties that don't exist in the config schema. That said, it's an improvement on the read side in the sense that any property you read is typed--even unknown keys default to
undefined.This comes with two potential breaking changes; I didn't find that they actually broke anything in practice, but they're worth looking out for in review:
users[id: string].authdefaults to an object. This could only break if a code path assumes e.g. the presence ofauth.githubmeans the user is authenticated; however, the way the types are structured marks all properties on that object as undefined, so this is a difficult error to make now.users[id: string].idis set. Failing to set it would make looking up user configuration impossible (we index into this object using the top-leveluserIdfield), so this prevents a class of mysterious logic errors.Stronger typing caused some issues in tests that mock the global config store; broke the
GlobalConfigStore's storage logic out into an adapter and implemented an in-memory adapter for use in tests. There are better ways to solve this problem--tests should be able to mock the global config without resorting to module mocking--but this will do for now.I think the global config store interface could use some work: at this point I think the dot-prop style of setting and getting properties only adds complexity vs something like a Proxy-wrapped object. That said, this is a step forward without committing to a bigger change.