-
Notifications
You must be signed in to change notification settings - Fork 23
perf: oas-form bloat reduction and bundle overhauls #1179
Conversation
2fae577
to
1c7f4ce
Compare
f1ef333
to
ab4705f
Compare
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 work looks great! From my once-over:
- Does the explorer throw any erorrs?
Randomly poked through our list of OAS schemas in the CI app—most all of 'em rendered great! I did come across one or two errors, but these endpoints are also on the fritz in "production" (on preview.readme.io, that is) so I doubt it's anything specific to this PR! - Does polymorphism still work?
I checked out the polymorphism example and played around with various bits and pieces: updating data in the form, futzing with the JSON editor, scanning the req/res snippets, etc. E'erything seemed in good working order to my eye. - Does form data updating still work?
Yep, at least from what I know to test! Snippet generation looks good. JSON editor reset is working nicely.
All that said, I'm an OAS noob so I wouldn't be surprised if I missed some deeper, mission-critical functionality... 😶 Will leave the rest of these tests to those more qualified!
-
Does Try It still work? -
Do discriminators still work?
@rafegoldberg which ones did you find errors on? |
@erunion there were two if I remember right, but this is the only one I still have the URL for on my clipboard. |
this.state = { | ||
selectedOption: this.getMatchingOption(formData, options), |
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.
Do we ever provide form data to an initial page load? If so, losing this will stop the polymorphism dropdown from having an initially selected value
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.
Thankfully we don't. Only data we provide is through JSON Schema default
props, and those get automatically compiled into the initial page load through computeDefaults
.
@@ -246,9 +242,6 @@ function computeDefaults(_schema, parentDefaults, rootSchema, rawFormData = {}, | |||
} | |||
|
|||
return computeDefaults(refSchema, defaults, rootSchema, formData, includeUndefinedValues); |
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.
Discriminators uses this, but I never really bothered to check what it was for? If the discriminator tests run I probably feel fine with this change.
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 compiles the original default formData
on load.
@@ -691,124 +648,6 @@ export function retrieveSchema(schema, rootSchema = {}, formData = {}) { | |||
return resolvedSchema; | |||
} | |||
|
|||
function resolveDependencies(schema, rootSchema, formData) { |
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.
What are dependencies and why don't we need this?
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.
That's all from this https://react-jsonschema-form.readthedocs.io/en/latest/usage/dependencies/. It's not part of the JSON Schema spec anymore.
🧰 What's being changed?
core-js
and instead use our core Babel config when building the dist for@readme/oas-form
.@babel/preset-env
for handling polyfills, we don't need to also load incore-js
for the same (and also we were only loading that in for a polyfill ofArray.includes
andArray.fill
).lib/
directory for@readme/oas-form
. It's holdover from RJSF and we use it, need it, nor do we need to keep the Babel infrastructure in this package to maintain it.rawErrors
, anderrorSchema
props as well as theonError
event handler.dependencies
fromoas-form
.dependencies
at all. OAS 3.1 supports a flavor of this throughdependentSchemas
, but we aren't there yet to be able to adequately support this so I'm pulling it as it adds a chunk of unused bloat.omitExtraData
andliveOmit
props.formData
whenonSubmit
is called but as we neither set these totrue
or have any cases where form data would be present without having gone through the form it doesn't make sense to keep it.autocomplete
prop.autocomplete
andautoComplete
prop. The former is deprecated for the latter. We use neither, but being able to extend that down to theForm
component is nice and we might use it in the future so I'm removing the deprecated version.withTheme
export.Form
component, but since we just use this stuff directly we don't need it around. Dropping it also allows us to remove the dependency onreact-is
for detectingReact.forwardRef
components.ui:order
UI schema property for customizing the order of object properties.union
,pick
,get
, andisEmpty
methods.🐳 Why remove AJV and validation support?
The amount of overhead for what AJV does, validate JSON Schema, is incredible compared to what we actually need it to do: validate form inputs.
Since we don't need to use AJV to validate that arrays are arrays, or objects are objects, because those schemas are already handled by way of the form construction, what's left for it to validate is schema patterns. Well thankfully input patterns are natively supported on every browser that we target: https://caniuse.com/input-pattern. Now while we can't natively render multiple errors on inputs at the same time, we can use native APIs to compile those errors together and render them somewhere (as demonstrated here).
That said, since we aren't even doing validation right now, and don't plan to do it with AJV in the future, it doesn't make sense to ship all this unused bloat right now and for the indefinite future.
🧹 Bundle size changes
@readme/api-explorer
@readme/oas-form
next
)But wait, how come removing AJV didn't clear off hella space on the bundle? Well the short of it is that there's still a lot of packages that we're loading into the bundle that we shouldn't be; namely
@readme/syntax-highlighter
:🧙 Future
There are still a lot of improvements that we can make to further reduce the size of our API Explorer bundles:
@readme/syntax-highlighter
as an external. Unfortunately this isn't that easy because@readme/ui
and@readme/oas-to-snippet
load it in so we'd need to make some improvements there.@readme/ui
is deprecated we should pull in the components that we use there into this library. This'll allow to exclude all components, compositions, and views that we don't use.@apidevtools/json-schema-ref-parser
? It has support for parsing YAML files, but since ouroas
library, which loads it for dereferencing, only handles JSON, it doesn't make much sense to ship full copies ofjs-yaml
andesprima
in our bundles.🧬 Testing