-
Notifications
You must be signed in to change notification settings - Fork 83
feat: Added ODP Segment Manager Implementation #790
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
369ac65
to
0af3793
Compare
@jaeopt / @zashraf1985 - updated and ready for review now. |
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.
Looks good! A couple of restructuring/renaming requests. They can be addressed here or in other PRs.
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.
Looks Good overall, requested a few minor changes here and there. I have a question about the location of these classes though. I believe, we kept those things in plugins which will be injected from the entry points based on the platform. The OdpOption and OdpSegmentManager are core classes which are independent of the platforms. These can go inside core
in my opinion.
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.
LGTM
Summary
JavaScript implementation of the ODP segment manager.
Provides an internal API for fetching segments.
Includes OdpConfig, a class that represents a configuration used for ODP integration, and OdpOption, an enum that can hold one of two ODP option values for cache handling.
Additionally this PR includes a refactor of several previously implement ODP components including LRU Cache, the ODP Segment API Manager (previously GraphQL Manager), the ODP Events API Manager (previously Rest API Manager), etc. to proper folders and with refocused naming conventions as discussed with @jaeopt and @zashraf1985
Test plan
Issues