Skip to content
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

fix(config): cast configuration values into proper types #9829

Merged
merged 9 commits into from
Apr 18, 2024
Merged

Conversation

glowcloud
Copy link
Contributor

@glowcloud glowcloud commented Apr 16, 2024

Refs #9808
Supersedes #9722

@char0n
Copy link
Member

char0n commented Apr 17, 2024

TODO:

  • align with the documentation
  • align swagger-ui-react defaultProps

@glowcloud
Copy link
Contributor Author

glowcloud commented Apr 17, 2024

I went through the docs and code to check for types and default values, skipping functions. Compiled the places where we don't specify what the default is or where there are differences in types.

OPTION DOCS DEFAULT REACT DEFAULT REACT PROP TYPE TYPE CASTER
dom_id string null null or string
domNode Element null null or value
spec object {} “” string or object object
urls array null null or array
layout string BaseLayout BaseLayout String
oauth2redirectUrl string string undefined string undefined or string
withCredentials boolean undefined undefined boolean undefined or boolean
  • dom_id and domNode - we're not mentioning that they can be null, just that one of them is required if the other is not specified
  • spec not sure if we should change the docs to allow string or React prop types to only allow object, or just leave them as is and create a type caster for it
  • urls - we're just not mentioning that the default is null
  • layout - not casting this as it's different in storeOptions:
    layout: {
    layout: options.layout,
    filter: options.filter,
    },
    so I think we would need to pass an additional flag on which options we're merging - otherwise we can't know if the layout can be an object or not
  • oauth2redirectUrl - we're just not mentioning that it can be undefined
  • withCredentials - we're not mentioning that the default is undefined and I'm also wondering if it shouldn't just be false. We have this in Swagger UI:
  const value = system.getConfigs().withCredentials

  if(value !== undefined) {
    system.fn.fetch.withCredentials = value
  }

and in Swagger Client:

const credentials = http && http.withCredentials ? 'include' : 'same-origin';
const credentials =
  this.getHttpClient().withCredentials || this.withCredentials ? 'include' : 'same-origin';
const credentials = http.withCredentials ? 'include' : 'same-origin';

But for each of them, it looks to me like if the value was false, it would work the same way. But perhaps we also check it somewhere else.

@glowcloud
Copy link
Contributor Author

Compiled the places where we don't specify what the default

I think I understand now - the docs actually just simply don't mention the default value if the default is null, undefined or empty string.

@char0n
Copy link
Member

char0n commented Apr 18, 2024

I think I understand now - the docs actually just simply don't mention the default value if the default is null, undefined or empty string.

We'll accept and following this further.

@char0n
Copy link
Member

char0n commented Apr 18, 2024

layout - not casting this as it's different in storeOptions:

We'll use naked deep-extend here.

Copy link
Member

@char0n char0n left a comment

Choose a reason for hiding this comment

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

Looks good. Nicely done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants