-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat[gen2]: ENG-11602 add support for global symbols #4307
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
base: main
Are you sure you want to change the base?
Changes from all commits
840bb2f
48135ef
9214940
dc4422b
7657fe4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -132,7 +132,7 @@ export default function Symbol(props: SymbolProps) { | |
| nonce={props.builderContext.value.nonce} | ||
| isNestedRender | ||
| apiVersion={props.builderContext.value.apiVersion} | ||
| apiKey={props.builderContext.value.apiKey!} | ||
| apiKey={(props.symbol?.global && props.symbol?.ownerId) ? props.symbol.ownerId : props.builderContext.value.apiKey!} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we check if the ownerId is part of the same root org ? Else it will allow global symbol to be used between orgs right ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good suggestion, but I don't think there is a way of knowing the root org at SDK side (do correct me if I am wrong), and anyways the UI and backend already have those checks to control which symbols are insertable to which spaces, so it should not be concerning. |
||
| context={{ | ||
| ...props.builderContext.value.context, | ||
| symbolId: props.builderBlock?.id, | ||
|
|
||
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.
Duplicate
SymbolInfointerface risks future divergenceLow Severity
The
SymbolInfointerface is independently defined in bothsymbol.helpers.tsandsymbol.types.tswith identical fields. This PR addsownerIdandglobalto both copies. The component passesprops.symbol(typed viasymbol.types.ts) tofetchSymbolContent(typed viasymbol.helpers.ts), so if these definitions ever diverge, type mismatches could go unnoticed.symbol.helpers.tscould importSymbolInfofromsymbol.types.tsinstead of redeclaring it.Additional Locations (1)
packages/sdks/src/blocks/symbol/symbol.types.ts#L7-L17