Skip to content
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

chore: Support react-native auto env #354

Closed

Conversation

yusinto
Copy link
Contributor

@yusinto yusinto commented Jan 24, 2024

Big PR for react native because:

  • We are adding auto env support. The main logic is here.
  • The RN SDK now needs Platform.Crypto to be implemented.
  • Mocks and tests must be modified. Note server sdk related changed are side-effects of the mocks api changing. There's no functionality changes to server related code.

yusinto added 30 commits January 4, 2024 12:10
…ored common properties to its own interface.
…moved LDAutoEnv interface. Remove auto env from LDUser and LDSingleKind.
… Removed redundant exclusion for setupFilesAfterEnv.
Comment on lines +11 to +15
// TODO: populate application ID, name, version, versionName
id: '',
name: '',
version: '',
versionName: '',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still unsure how to populate these without installing the heavyweight react-native-device-info or having native dependencies. We may have to document this to request customers to set the application tags for these to be correctly populated.

Copy link
Member

Choose a reason for hiding this comment

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

If we cannot collect, then we will want to exclude them when they are not populated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to platform/crypto for posterity.

digest(encoding: SupportedOutputEncoding): string {
switch (encoding) {
case 'base64':
return base64FromByteArray(new Uint8Array(this.hasher.arrayBuffer()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The js-sha256 library supports hex output but not base64. I'm doing it manually here.

Comment on lines +63 to +64
info: setupInfo(),
crypto: setupCrypto(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I encountered mock issues where mock resets don't work with static exports in deep nested mocks. Therefore I have to make these changes and introduce a flagfall test setup step for the mocks package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be run beforeEach test for basicPlatform mocks to be setup and initialized correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file seems unused and its removal does not break anything.

Comment on lines -12 to -26
const { isLegacyUser, isMultiKind, isSingleKind } = internal;

export const addNamespace = (s: string) => `LaunchDarkly_AnonKeys_${s}`;

export const getOrGenerateKey = async (kind: string, { crypto, storage }: Platform) => {
const nsKind = addNamespace(kind);
let contextKey = await storage?.get(nsKind);

if (!contextKey) {
contextKey = crypto.randomUUID();
await storage?.set(nsKind, contextKey);
}
import { getOrGenerateKey } from './getOrGenerateKey';

return contextKey;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are moved to a separate file getOrGenerateKey.ts.

Comment on lines +20 to +27
export const toMulti = (c: LDSingleKindContext) => {
const { kind, ...contextCommon } = c;

return {
kind: 'multi',
[kind]: contextCommon,
};
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converting a single kind to multi kind. Is this logic correct and sufficient enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All remaining changes below this comment are related to the mocks api being changed due to basicPlatform needing to be setup on beforeEach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main auto env logic.

Comment on lines +75 to +78
// LDUser is not supported for auto env reporting
if (isLegacyUser(context)) {
return context as LDUser;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Call out we are not supporting LDUser for auto env.

Comment on lines +42 to +45
ldApplication.id = config.application?.id || ldApplication?.id || name || wrapperName;
ldApplication.name = ldApplication?.name || name || wrapperName;
ldApplication.version =
config.application?.version || ldApplication.version || version || wrapperVersion;
Copy link
Contributor Author

@yusinto yusinto Jan 24, 2024

Choose a reason for hiding this comment

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

I follow the spec logic, but it does not specify how application name should be populated. Is this acceptable?

ldApplication.name = ldApplication?.name || name || wrapperName;

export const ldDevice: LDDevice = {
key: '',
envAttributesVersion: '1.0',
manufacturer: Platform.select({
Copy link
Member

Choose a reason for hiding this comment

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

Pinging @tanderson-ld on these values.

@yusinto Do you have the JSON of one of the contexts populated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is from the ios simulator:

{
  "kind": "multi",
  "user": {
    "key": "test-user"
  },
  "ld_application": {
    "key": "/zFu3yVDTWyKZ6vG+ssjgcM9uSTmTpD2u/QTEy2FFVY=",
    "envAttributesVersion": "1.0",
    "id": "@launchdarkly/react-native-client-sdk",
    "name": "@launchdarkly/react-native-client-sdk",
    "version": "0.1.5",
    "versionName": "",
    "locale": "en_US"
  },
  "ld_device": {
    "key": "21105df8-0236-4ef5-9721-f94f68424560",
    "envAttributesVersion": "1.0",
    "manufacturer": "apple",
    "os": {
      "family": "apple",
      "name": "ios",
      "version": "17.2"
    }
  }
}

};

export const ldDevice: LDDevice = {
key: '',
Copy link
Member

Choose a reason for hiding this comment

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

I assume this gets populated later, maybe a comment to explain?

name: 'An OS',
version: '1.0.1',
arch: 'An Arch',
name: 'iOS',
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? It is good for this data to not look like real runtime data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason. I can revert all good.

Copy link
Member

@kinyoklion kinyoklion left a comment

Choose a reason for hiding this comment

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

This PR contains changes that require common release, for instance, so it cannot be a chore.

I think we should break this PR down into some units with some associated conventional commits that will make them digestable.

feat: Implement common support for auto environment attributes.
feat: Implement common client side support for auto environment attributes.
feat: React-native support for auto-env attributes.

Each PR affecting the correct components to produce the generally correct changlog history.

If there are some sweeping changes for mocks or jest config, then those could be done in a chore.

@kinyoklion
Copy link
Member

Overall I think things are looking good. I do think we need to break it up though and apply it in smaller bits that can be released and have associated change notes.

@yusinto
Copy link
Contributor Author

yusinto commented Jan 25, 2024

we need to break it up
feat: Implement common support for auto environment attributes.

#355

feat: Implement common client side support for auto environment attributes.

#356

feat: React-native support for auto-env attributes.

#357

yusinto added a commit that referenced this pull request Jan 25, 2024
This PR adds support for auto env attributes for the common library.
This was originally part of #354. That big PR is being broken down into
smaller PRs for the benefit of better changelog through conventional
commits.

Note I have intentionally omitted the `common/jest.config.js` change
because that is part of a larger change to the mocks api. This change
is in #356.
@yusinto
Copy link
Contributor Author

yusinto commented Feb 1, 2024

Closing this because it has been broken up and superseded by #356 #357 #358.

@yusinto yusinto closed this Feb 1, 2024
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.

2 participants