Skip to content

SDKS-4801#27

Merged
tsdamas merged 33 commits into
mainfrom
SDKS-4556
Apr 9, 2026
Merged

SDKS-4801#27
tsdamas merged 33 commits into
mainfrom
SDKS-4556

Conversation

@tsdamas

@tsdamas tsdamas commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

JIRA Ticket

SDKS-4801: Address CI failures and refactor logger integration

Description

Removes configureLogger from the public API and simplifies the logger integration contract across all SDK packages. Key changes:

  • Dropped nativeLogger/nativeHandle fields from all package config types (storage, journey, oidc, browser, device-profile)
  • Removed configureLogger from the sample app, integration tests, and docs
  • Updated native Android/iOS logger integration in browser and device-profile
  • Added/fixed tsconfig.build.json across all packages to resolve rn-types path alias to built declarations, preventing typecheck errors on cold rebuilds
  • Added ^build to turbo.json typecheck dependencies to ensure packages are built before typechecking
  • Migrated RNPackagesTests Xcode scheme from PingSampleApp to PingTestRunner
  • Excluded lib/ from Jest haste map in all package configs
  • Updated Mend SAST config for private repo compatibility

Note

For reviewers: The diff is large but the majority of changes across PingSampleApp and package source files are automated Prettier and ESLint formatting fixes (quote style, trailing commas, spacing, semicolons). Focus review effort on the logical changes in packages/logger/src/, the config type deletions across each package's types/ folder, and the tsconfig.build.json additions.

Checklist:

  • I ran all unit tests and they pass

@tsdamas tsdamas changed the title SDKS-4556 (do not review) SDKS-4556 WIP Mar 27, 2026
@github-actions

github-actions Bot commented Mar 27, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://ForgeRock.github.io/ping-react-native-sdk/docs-preview/pr-27/

Built to branch gh-pages at 2026-04-09 21:04 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

tsdamas added 20 commits March 27, 2026 14:35
….json. Override the @ping-identity/rn-types path alias in tsconfig.build.json for all packages to use lib/typescript/src/index instead of the source path. This prevents TS6059 rootDir errors when the types package cache is busted and packages rebuild from scratch.
…kages are built before PingSampleApp typechecks and remove mend sast configExternalUrl for private repo compatibilitty
@tsdamas tsdamas changed the title SDKS-4556 WIP SDKS-4801 Mar 31, 2026
Comment thread packages/types/src/index.d.ts.map Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like a build artifact. Do we need to commit .map files?

@tsdamas tsdamas Apr 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, no, those files don’t need to be committed. They were generated build artifacts that slipped into the PR. I’ll clean them up and update the ignore rules if needed.

@pingidentity-gaurav

Copy link
Copy Markdown
Contributor

LGTM

@rodrigoareis rodrigoareis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall changes looks good. Left some minor comments.

Comment thread packages/storage/src/index.tsx Outdated

/**
* Cached default logger used when callers do not provide one.
*/
let defaultLoggerInstance: LoggerInstance | null = null;

const createNoopLogger = (): LoggerInstance => ({
nativeHandle: { id: "native-none-id" },
nativeHandle: { id: 'native-none-id' },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems the storage module is not aligned with other packages changes. It was not updated to match the simplified noop pattern used in browser, device-profile, journey, and oidc.

return { module, nativeLogger, sdkLogger };
};

describe('logger package', () => {
it('configureLogger registers a native logger and returns a handle', async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These tests for configureLogger were removed. But since configureLogger is still called internally by logger(), the error path — where registerLogger returns an empty string — is no longer tested.

Also, now it's missing test for default log level.

Comment thread packages/device-id/tsconfig.json Outdated
@@ -26,5 +26,6 @@
"strict": true,
"target": "ESNext",
"verbatimModuleSyntax": true
}
},
"exclude": ["lib"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"exclude": ["lib"]
"exclude": ["lib", "**/__tests__/**"]

Comment thread packages/device-profile/tsconfig.json Outdated
@@ -26,5 +26,6 @@
"strict": true,
"target": "ESNext",
"verbatimModuleSyntax": true
}
},
"exclude": ["lib"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"exclude": ["lib"]
"exclude": ["lib", "**/__tests__/**"]

Comment thread packages/journey/tsconfig.build.json Outdated
"@ping-identity/rn-journey": ["./src/index"],
"@ping-identity/rn-types": ["../types/lib/typescript/src/index"]
}
},
"exclude": ["example", "lib"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"exclude": ["example", "lib"]
"exclude": ["lib", "**/__tests__/**"]

@@ -4,7 +4,8 @@
* This software may be modified and distributed under the terms
* of the MIT license. See the LICENSE file for details.
*/
/* eslint-env jest */

import packageJson from '@ping-identity/rn-logger/package.json';

export {};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it can be removed, the file has a top-level import packageJson

* - Handles native errors gracefully
*/

export {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
export {}
export {};

* - Propagates native errors to callers
*/

export {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
export {}
export {};

* - Propagates native errors to callers
*/

export {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
export {}
export {};

* - dispose() cleans up the native instance
*/

export {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
export {}
export {};

@rodrigoareis rodrigoareis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@tsdamas tsdamas merged commit 070940d into main Apr 9, 2026
7 of 10 checks passed
@tsdamas tsdamas deleted the SDKS-4556 branch April 9, 2026 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants