Core - renaming public APIs #91

yangyansong-adbe · 2021-11-19T22:22:03Z

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

* add a new API in Core -? resetIdentities * update core package version to alpha.2

* commit 'b58470acfaf35b5dc6abdabc413233d609de9a51': EdgeIdentity (1.0.0-alpha.2) -> main (adobe#62) # Conflicts: # apps/AEPSampleApp/ios/Podfile.lock

…inning October 4, 2021 (adobe#73)

* staging: fix the release issue: NPM registry enforces a minimum of TLS 1.2 beginning October 4, 2021 (adobe#73) add a FAQ section (adobe#66) Update README.md add a new API in Core - resetIdentities (adobe#63) (adobe#64) EdgeIdentity (1.0.0-alpha.2) -> main (adobe#62)

* commit 'ca66509d5cf215d594be6b9eeb894cb3c3d913cd': Merge latest staging changes (adobe#85) add a new API in Core - resetIdentities (adobe#63) # Conflicts: # apps/AEPSampleApp/ios/Podfile.lock # yarn.lock

…aming * commit '801cf01b932470064b8f358784b589f4b3c5bbd4': Reduce visibility for utility classes and handle error callback (adobe#86) # Conflicts: # packages/core/android/src/main/java/com/adobe/marketing/mobile/reactnative/RCTAEPCoreDataBridge.java # yarn.lock

apps/AEPSampleApp/extensions/CoreView.js

apps/AEPSampleApp/extensions/EdgeIdentity.js

packages/core/README.md

packages/core/js/index.d.ts

emdobrin · 2021-11-22T03:14:25Z

packages/core/js/MobileCore.js

   * @return true if the the event dispatching operation succeeded, otherwise the promise will return an error
   */
-  dispatchEvent(event: AEPExtensionEvent): Promise<boolean> {
+  dispatchEvent(event: Event): Promise<boolean> {


This API can be renamed to dispatch(event: Event)

I think dispatchEvent is a better option for the loosely typed JavaScript. We also use this method name in Android SDK ->

public static boolean dispatchEvent(final Event event, final ExtensionErrorCallback<ExtensionError> errorCallback)

emdobrin · 2021-11-22T03:16:15Z

packages/core/js/MobileCore.js

   * @see AEPCore#dispatchResponseEvent(Event, Event, ExtensionErrorCallback)
   */
-  dispatchEventWithResponseCallback(event: AEPExtensionEvent): Promise<AEPExtensionEvent> {
+  dispatchEventWithResponseCallback(event: Event): Promise<Event> {


can be renamed to: dispatchWithResponseCallback..

Unrelated to your changes, but I noticed this API overwrites the timeout to 1 sec in the objc implementation. Is this the desired behavior or can we fallback to the default in the native extension?

https://github.com/adobe/aepsdk-core-ios/blob/daa7245798d99b9cc85f827a125d069aa7a54c29/AEPCore/Sources/core/MobileCore.swift#L118
1 second is the default timeout value of AEPCore API. We didn't overwrite it anyway.

packages/core/js/MobileCore.js

emdobrin · 2021-11-23T18:43:55Z

packages/core/js/Identity.js

+   * @see PrivacyStatus
   */
-  syncIdentifiersWithAuthState(identifiers?: {string: string}, authenticationState: string) {
+  syncIdentifiersWithAuthState(identifiers?: { string: string }, authenticationState: string) {


Any reason why authenticationState is not of type MobileVisitorAuthenticationState ? Same for the APIs below

https://github.com/adobe/react-native-acpcore/blob/05a9b2ed10ccec9e7ae1693913884eaa03c6e060/js/ACPIdentity.js#L67
I just followed what we did in ACPIdentity wrapper. I'm not familiar with Identity extension, please feel free to caret a new PR to fix this issue.

In my opinion we can use the enum here for the AEP wrapper. Would you revisit this in the final review of the Core extensions?

Emm ... I think we'd better let our Identity experts fix this issue in another PR.

Sounds good, let's revisit this in a separate PR, I created #96

emdobrin · 2021-11-23T18:48:48Z

packages/core/js/models/LogLevel.js

 const ERROR = "AEP_LOG_LEVEL_ERROR";
 const WARNING = "AEP_LOG_LEVEL_WARNING";
 const DEBUG = "AEP_LOG_LEVEL_DEBUG";
 const VERBOSE = "AEP_LOG_LEVEL_VERBOSE";


can we rename all the constants too? e.g. AEP_LOG_LEVEL_VERBOSE -> VERBOSE
This comment applies for all enums

I think we'd better keep those AEP information, then it should be easier to identify wrapper logs in react native console.

If the usage looks like LogLevel.VERBOSE or PrivacyStatus.AUTHENTICATED I feel the string constant will be more consistent with the usage that way if we don't have any other limitations.

I agree an Enum should be a better fit here. I don't know if there is any limitation to it. Can you create a GitHub issue for it? I need to figure out why we use String instead of Enum here, then we can decide what we can do later.

emdobrin

👍 Looks with with some small comments

emdobrin · 2021-11-24T23:07:36Z

packages/core/js/models/MobileVisitorAuthenticationState.js

 const LOGGED_OUT = "AEP_VISITOR_AUTH_STATE_LOGGED_OUT";
 const UNKNOWN = "AEP_VISITOR_AUTH_STATE_UNKNOWN";


These ones need updates too

emdobrin · 2021-11-24T23:08:59Z

apps/AEPSampleApp/App.js

      <Button
        onPress={() => navigation.navigate('Identity')}
        title="Identity"
      />


L17 requires update to navigate('CoreView')

emdobrin · 2021-11-24T23:11:57Z

apps/AEPSampleApp/extensions/IdentityView.js


 function syncIdentifiersWithAuthState() {
-  AEPIdentity.syncIdentifiersWithAuthState({"id1": "identifier1"}, "AEP_VISITOR_AUTH_STATE_AUTHENTICATED");
+  Identity.syncIdentifiersWithAuthState({ "id1": "identifier1" }, "AEP_VISITOR_AUTH_STATE_AUTHENTICATED");


we can use MobileVisitorAuthenticationState.AUTHENTICATED

emdobrin · 2021-11-24T23:17:22Z

packages/core/js/Identity.js

+   * @see PrivacyStatus
   */
-  syncIdentifiersWithAuthState(identifiers?: {string: string}, authenticationState: string) {
+  syncIdentifiersWithAuthState(identifiers?: { string: string }, authenticationState: string) {


Sounds good, let's revisit this in a separate PR, I created #96

yangyansong-adbe added 14 commits September 22, 2021 20:10

EdgeIdentity (1.0.0-alpha.2) -> main (adobe#62)

b58470a

add a new API in Core -? resetIdentities

c552d1f

update core package version to alpha.2

192c573

add a new API in Core - resetIdentities (adobe#63) (adobe#64)

e67c005

* add a new API in Core -? resetIdentities * update core package version to alpha.2

Merge commit 'b58470acfaf35b5dc6abdabc413233d609de9a51' into staging

a09ae8e

* commit 'b58470acfaf35b5dc6abdabc413233d609de9a51': EdgeIdentity (1.0.0-alpha.2) -> main (adobe#62) # Conflicts: # apps/AEPSampleApp/ios/Podfile.lock

Update README.md

784c618

add a FAQ section (adobe#66)

f4001a5

fix the release issue: NPM registry enforces a minimum of TLS 1.2 beg…

21960e1

…inning October 4, 2021 (adobe#73)

reduce visibility for utility classes.

3123002

handle error callback

c647eb8

Merge commit 'ca66509d5cf215d594be6b9eeb894cb3c3d913cd' into core

4f96d1d

* commit 'ca66509d5cf215d594be6b9eeb894cb3c3d913cd': Merge latest staging changes (adobe#85) add a new API in Core - resetIdentities (adobe#63) # Conflicts: # apps/AEPSampleApp/ios/Podfile.lock # yarn.lock

rename public APIs

ab1ea9f

yangyansong-adbe added the core label Nov 19, 2021

fix sample app compile issue

a7f80d2

yangyansong-adbe requested review from cacheung, emdobrin and shtomar-adb November 19, 2021 23:09

emdobrin reviewed Nov 22, 2021

View reviewed changes

clean up logs, remove AEP prefix

4d96c2e

emdobrin reviewed Nov 23, 2021

View reviewed changes

yangyansong-adbe added 2 commits November 24, 2021 14:42

remove AEP prefix for Log tags

60015ef

remove AEP prefix from PriavacyStatus const strings

3307c41

emdobrin approved these changes Nov 24, 2021

View reviewed changes

yangyansong-adbe added 2 commits November 28, 2021 17:08

fix compile failure

4bca93b

Remove AEP prefix for internal constant strings.

8fb629e

yangyansong-adbe merged commit 638519e into adobe:core Nov 29, 2021

yangyansong-adbe deleted the core-renaming branch December 17, 2021 02:30

		const LOGGED_OUT = "AEP_VISITOR_AUTH_STATE_LOGGED_OUT";
		const UNKNOWN = "AEP_VISITOR_AUTH_STATE_UNKNOWN";

Core - renaming public APIs #91

Core - renaming public APIs #91

Uh oh!

Conversation

yangyansong-adbe commented Nov 19, 2021

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangyansong-adbe Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emdobrin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yangyansong-adbe Nov 23, 2021 •

edited

Loading