Skip to content

Conversation

@kinyoklion
Copy link
Member

@kinyoklion kinyoklion commented May 26, 2022

This adds the basic implementation of attribute references and parsing. It does not add attribute redaction for event filtering. There will be some changes once this is added.

The general approach here is somewhere between the typed and untyped SDKs. Get things uniform enough to simplify evaluation, but do not comprehensively copy and evaluate them. This is to try to reduce the work that needs to be done on the hot path.

Because this code would be common to client and server SDKs it is in sdk-common. I also moved the validation code over so it could be used there as well as sever.

kinyoklion and others added 30 commits May 19, 2022 16:26
Co-authored-by: Alex Engelberg <alex.benjamin.engelberg@gmail.com>
@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #154351: Implement context type and attribute references..

/**
* If true, the context will _not_ appear on the Contexts page in the LaunchDarkly dashboard.
*/
transient?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was in the wrong place. Originally it was _meta and it looks like I forgot to update that after it changed.

);
if (index < 0) {
throw new Error(`Did not find expected message: ${expectedMessage}`);
throw new Error(`Did not find expected message: ${expectedMessage} received: ${this.messages}`);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this on the other PR.

"build": "tsc"
},
"license": "Apache-2.0",
"dependencies": {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the server common is using common.

@@ -1,4 +1,4 @@
import { LDContext } from './LDContext';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the context related interfaces were moved to common.

* For now this is for testing and therefore is flagged internal.
* It will not be accessible outside this package.
*
* @internal
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may change depending on where the filtering goes. Either becoming part of the interface, or getting further encapsulated. It is nice to be able to access these for a quick sanity check in the tests though.


private contexts: Record<string, LDContextCommon> = {};

private privateAttributeReferences?: Record<string, AttributeReference[]>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did decide to pre-process attribute references. They aren't as unbounded as doing a full conversion. I may change it to be lazy if the performance is a problem.

@kinyoklion kinyoklion changed the title Add attribute references and context. #1 Add attribute references and context. May 27, 2022
@kinyoklion kinyoklion requested a review from yusinto May 31, 2022 19:45
kinyoklion and others added 2 commits June 1, 2022 09:24
Co-authored-by: Matthew M. Keeler <keelerm84@gmail.com>
Co-authored-by: Matthew M. Keeler <keelerm84@gmail.com>
@kinyoklion kinyoklion merged commit 9bd8afe into main Jun 1, 2022
@kinyoklion kinyoklion deleted the rlamb/sc-154351/attributes-and-context branch June 1, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants