-
Notifications
You must be signed in to change notification settings - Fork 7
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
refactor types w/ instance options system #175
Conversation
🦋 Changeset detectedLatest commit: 0e112f2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
✅ Deploy Preview for dmno ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for signup-api canceled.
|
commit: |
5d3db05
to
eea2e1e
Compare
…ent, add allowMissing to ctx.get, add git vendor types autopopulate
eea2e1e
to
0e112f2
Compare
resolverCtxAls.enterWith(itemResolverCtx); | ||
|
||
|
||
if (this.valueResolver) { | ||
if (this.overrides.length) { |
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.
previously we were still running the resolver, and would still surface resolution errors if there were any. This now skips the resolution instead - both removing the error, and potentially speeding things up.
const node = this.entity?.getConfigNodeByPath(nodePath); | ||
if (!node) { | ||
if (opts?.allowMissing) return; |
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.
if allowMissing
option is toggled on, it wont throw an error anymore. So far this is useful when defining a vendor schema and you have a computed value, and you dont know if the user will pick all of them. I can imagine other cases too.
...gitCommonTypeInfo, | ||
}), | ||
BranchName: createDmnoDataType( | ||
(opts?: { |
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 is what prompted the changes. I wanted to be able to toggle having a built-in value
(resolver) based on the presence of an instance option.
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.
🚢
While working through some hairy type issues, I noticed simplification of how the type system works with regards to types that have specific instance options. Previously we were attaching those options to the DmnoDataType instance and then making them available on
this
during coercion and validation.I wanted the settings to be available all the time, and realized I overlooked just using a function in a slightly different way.
End users will not notice anything. This may open the door for further simplification later, but for now it was easier to just leave things mostly as is.
Worked in a few related changes as well
allowMissing
option to ctx.get - useful within vendor schemas where another node might exist