-
Notifications
You must be signed in to change notification settings - Fork 347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: provide config extends
feature
#2222
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this pull request and this general feature seems useful!
However, we're aiming to be conservative with the number of external dependencies we take, and config management hasn't been enough of a pain point that we want to outsource this logic yet.
To me, it seems like we need a way to fetch the remote files (included in this PR) and a deepMerge
implementation (~15 LoC, rather than the external dependency).
src/util/http-api.ts
Outdated
|
||
import * as https from 'https'; | ||
|
||
// Hmmm... no-experimental-fetch flag is used. Ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flag is used in test because we've been using nock
to fake http requests and it can't handle the built-in fetch :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to mock the transport layer? Maybe http.getJson
mock would suffice here?
@@ -0,0 1,7 @@ | |||
{ | |||
"extends": "owner/repo/main", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps:
owner/repo@branchOrSha:path/to/file.json
owner/repo/path/to/file.json@branchOrSha
owner/repo:path/to/file.json@branchOrSha
Or something similar to https://docs.renovatebot.com/config-presets/#github
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can combine. I've added support for the renovate-like format and for the last two options.
I understand... But once the 15loc deepmerge meets the prod area, it turns into 115loc deepmerge. And another 1115 lines of tests. I know this road; I have walked along it many many times. I'd even suggest using yargs/helpers#applyExtends here, but unfortunately there is no way to override its internal loader. Dependencies are always a trade-off between complexity, risks and implementation costs. For example, someone's reinventing cosmiconfig in place (ref1, ref2, ref3), while deepmerge itself doesn't answer what to do with |
This PR provides config extensions with github-hosted assets:
Fixes #2048 🦕
Fixes #1979