-
Notifications
You must be signed in to change notification settings - Fork 220
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
Changes to support the ALLCAPS spec change in XVIZ #342
Conversation
treetable: XVIZTable, | ||
|
||
// Normalize enums to uppercase | ||
CONTAINER: XVIZContainer, |
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 technically public API (users can override default components with their own). Can we cast component type to lowercase in _renderItem
instead?
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 am doing this just for the default components then OK.
streamMetadata[streamName] && streamMetadata[streamName].primitive_type === 'image' | ||
streamMetadata[streamName] && | ||
streamMetadata[streamName].primitive_type && | ||
streamMetadata[streamName].primitive_type.toUpperCase() === 'IMAGE' |
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.
primitive_type === 'image' || primitive_type === 'IMAGE'
is more efficient.
@@ -217,7 +217,7 @@ test('XVIZStreamLoader#connect, seek', t => { | |||
loader.onXVIZMessage({type: LOG_STREAM_MESSAGE.METADATA, start_time: 1000, end_time: 1030}); | |||
t.deepEquals( | |||
socket.flush(), | |||
[{type: 'xviz/transform_log', data: {start_timestamp: 1000, end_timestamp: 1010, id: '0'}}], | |||
[{type: 'XVIZ/TRANSFORM_LOG', data: {start_timestamp: 1000, end_timestamp: 1010, id: '0'}}], |
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.
Just like aurora-opensource/xviz#477 I don't think we should change the type names.
This is dependent on aurora-opensource/xviz#477 and makes the expectation it will be published as beta.16. This cannot land until that is published.
This covers places in the code where any enumerated types where being referenced directly and will normalize them to UPPERCASE to support both existing data as well as new data. This should be backwards compatible with previously generated data but will have a hard dependency on the new xviz module version.
Testing was ran as follows:
yarn start-local
using local modules hitting AWS hosted datayarn start-streaming-local
using local modules hitting local XVIZ server with UPPERCASE datayarn start-local
using local modules hitting local XVIZ server with UPPERCASE datayarn start-local
using local modules and updated embedded test cases