-
Notifications
You must be signed in to change notification settings - Fork 0
chore/SOF-6428: build dependency tree #40
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
Conversation
| @@ -1,3 +1,5 @@ | |||
| import lodash from "lodash"; | |||
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.
It's better to load only lodash functions that we actually need in the module, not a whole lodash library.
https://stackoverflow.com/questions/35250500/correct-way-to-import-lodash
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.
good point! I adjusted it for all lodash imports in the repo
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.
@k0stik - the idea is good, provided that readability doesn't suffer, but let's deal with it in a separate task, pls see #40 (comment)
src/context/provider.js
Outdated
| * @notes Should hold static data only (see `setData` method), no classes or functions | ||
| */ | ||
| import lodash from "lodash"; | ||
| import capitalize from "lodash/capitalize"; |
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.
I understand where you're coming from on this, but I have to go with readability on this: lodash.get is way more understandable than get when reading the code. Plus, the 150.5KB size is not as important for us, given that the bundle size is measured in MBs. You can put a separate task for it, if you'd like.
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.
ok no problem, I can change it back
src/utils/schemas.js
Outdated
| ...getEnumValues(nodes), | ||
| }, | ||
| }; | ||
| forEach(mod, (extraFields, key) => { |
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.
Why not use the built-in Array.forEach?
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 due to calling forEach on an object. I can also implement it using Array.forEach where mod is a list of objects.
* chore: add draft for function to build dependency tree * chore: adjust dependency builder to support dataSelector node property * chore: add utility function for calling map on a tree * test: add test for buildDependencies * chore: add mapTree to utils index * chore: add function to determine schema type * chore: add function for building schema with dependency * refactor: buildDependencyCase function * test: getSchemaWithDependencies * test: typeofSchema * test: mapTree * docs: add docstring for getSchemaWithDependencies * test: adding enum dynamically to main schema * chore: add getSchemaWithDependencies to index * chore: use single lodash imports * revert: single lodash imports
This PR concerns an automatic way of building the dependency tree for RJSF schemas based on a given model subtree and a given schema reference.