-
Notifications
You must be signed in to change notification settings - Fork 178
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
feat(app): Get protocolDisplayData based on protocol schema #3531
Conversation
app/src/protocol/index.js
Outdated
@@ -108,6 +108,7 @@ type CreatorAppSelector = OutputSelector< | |||
> | |||
|
|||
const getName: StringGetter = getter('metadata.protocol-name') | |||
const getV3Name: StringGetter = getter('metadata.protocolName') |
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.
Kind of at a loss for what to name this - since we need old and new for now and V3 will become the standard soon?
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.
We're not dropping support for V1/V2 anytime soon AFAIK, and there will be future versions to co-support. Maybe something vaguely like:
const protocolV1V2GetterPaths = {
name: 'metadata.protocol-name'
...etc
}
const PROTOCOL_GETTER_PATHS_BY_SCHEMA = {
"1": protocolV1V2GetterPaths,
"2": protocolV1V2GetterPaths,
"3": {
name: 'metadata.protocolName',
...etc
}
}
const getProtocolSchemaVersion = getter('protocol-schema') || getter('schemaVersion') || null
// ...
const pathsForProtocol = getProtocolSchemaVersion(protocol)
const getName = getter(pathsForProtocol.name)
Codecov Report
@@ Coverage Diff @@
## edge #3531 +/- ##
=========================================
+ Coverage 27.61% 28.91% +1.3%
=========================================
Files 705 729 +24
Lines 11557 13086 +1529
=========================================
+ Hits 3191 3784 +593
- Misses 8366 9302 +936
Continue to review full report at Codecov.
|
050bcf6
to
4a42d20
Compare
- refactor: Switch to single getProtocolDisplayData selector
4a42d20
to
7fe8691
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.
got some nits but not blocking
shared-data/protocol/index.js
Outdated
export function getProtocolSchemaVersion(data: ProtocolData): ?number { | ||
if (data.schemaVersion) { | ||
return data.schemaVersion | ||
} else if (data['protocol-schema']) { |
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.
To be stricter about the "legacy" protocol-schema
key, we could do very explicit if (data['protocol-schema'] == '1.0.0') return 1; else if (data['protocol-schema'] == '2.0.0') return 2
. If it's '3lolz'
or even if it's 3.0.0
or 3
, those are invalid ways to designate a protocol as v3
I know this strictness is totally overkill for how this fn is used in this PR, but we're going to use this fn for protocol migration very soon and for that I'm not comfortable with it being this permissive
- Warn against schemas > 3 not yet supported
overview
This PR checks for protocol name, last modified, appName, and appVersion for based on protocol schema versions.
closes #3494
changelog
review requests
Labeled this a WIP for now - this issue is a little larger than expected as it touches more than just the name field once we start getting schema version from shared data. It also uncovered the lack of v3 protocol file flow type.
V2 protocol to test
V3 protocol to test