Feat: Use sandbox id by default for generation commands #669
Conversation
…not passed to the formgen command
sdstolworthy
requested review from
edwardfoyle,
sobolk and
Amplifiyer
as code owners
🦋 Changeset detectedLatest commit: be7c8a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -6,6 +6,13 @@ type BackendIdentifierParameters = { | |||
| appId?: string; | |||
| branch?: string; | |||
| }; | |||
|
|
|||
| export type BackendIdentityResolver = { | |||
There was a problem hiding this comment.
Having BackendIdentifierResolver and BackendIdentityResolver is confusing. It's difficult to know what the difference between each is. Could we have a single BackendIdentifierResolver type with two implementation: one that resolves a single type of BackendId and another that goes through the fallback chain?
If that proves to be too much refactoring, I think I'd rather just add the sandbox fallback directly to the existing BackendIdentifierResolver
There was a problem hiding this comment.
@edwardfoyle How do you feel about this? 2dc8035#diff-9db562c416fe5dbc758bce7b582f0b1c080d36a408d4e2b12c457f9bbc7645b3R20
| /** | ||
| * Resolves the backend id when branch or stack is passed as an arg, otherwise returns a sandbox backend identifier | ||
| */ | ||
| export class BackendIdentifierResolverWithFallback { |
There was a problem hiding this comment.
should this implement BackendIdentityResolver?
There was a problem hiding this comment.
see: #669 (comment)
| /** | ||
| * Resolves the backend id when branch or stack is passed as an arg, otherwise returns a sandbox backend identifier | ||
| */ | ||
| export class BackendIdentifierResolverWithFallback |
There was a problem hiding this comment.
nit:
| export class BackendIdentifierResolverWithFallback | |
| export class BackendIdentifierResolverChain |
Could even generalize to taking in an array of resolvers and return the first one that doesn't return undefined. But that could also be a load of YAGNI
There was a problem hiding this comment.
I went down that road last night and decided it was YAGNI and walked it back
| * resolves the backend id, falling back to the sandbox id if there is an error | ||
| */ | ||
| public resolve = async ( | ||
| ...args: Parameters<BackendIdentifierResolver['resolve']> |
There was a problem hiding this comment.
pretty sure this type will be inferred from the implementing interface. If not, just type it with BackendIdentifierParameters
There was a problem hiding this comment.
Ah yep now that this is implementing the interface, this should be removed.
| /** | ||
| * resolves the backend id, falling back to the sandbox id if there is an error | ||
| */ | ||
| public resolve = async ( |
There was a problem hiding this comment.
nit: public is the default
Issue #, if available:
Description of changes:
See #605 for previous comments.
This commit genericizes the Fallback resolver somewhat, accepting a default resolver and a fallback resolver.
Additionally,
BackendIdentifierResolver['resolve']can now return undefined, addressing the comment thread here: #605 (comment).By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.