-
Notifications
You must be signed in to change notification settings - Fork 281
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: Remove style-dictionary dependency from React Native package #5848
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 0eae19c The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -8,8 +8,10 @@ const config: Config = { | |||
'!<rootDir>/src/index.ts', | |||
// ignore internal `debugUtils` from coverage thresholds | |||
'!<rootDir>/**/debugUtils.ts', | |||
// ignore coverage for style-dictionary type declaration file | |||
'!<rootDir>/src/theme/types/style-dictionary.d.ts', |
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.
!<rootDir>/src/theme/types/style-dictionary.d.ts
does not exist anymore
@@ -0,0 +1,169 @@ | |||
// Internal Style Dictionary methods |
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.
Copied from [email protected]
which is specified in the ui package.json
}; | ||
let updated_object, regex, options; | ||
|
||
export function resolveObject<T>(object: Record<string, any>): T { |
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.
The only change to this file is adding the generic type <T>
on resolveObject
and traverseObj
. This is modeled after the way we copied deepExtend here and resolves type issues where these functions are used in the react native package.
@@ -0,0 +1,51 @@ | |||
// Internal Style Dictionary methods |
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.
Copied from [email protected] which is specified in the ui package.json, no changes made
@@ -0,0 +1,112 @@ | |||
import { has } from './utils'; |
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.
Copied from [email protected]. Only change is using our util has
in favor of hasOwnProperty
@@ -290,3 +290,23 @@ export function splitObject( | |||
}); | |||
return [left, right] as const; | |||
} | |||
|
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.
Copied from [email protected], did not change anything
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.
These changes make sense to me. Should be good once lint failures are resolved
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.
Thanks for doing this! Didn't get too deep but left some initiial feedback, additionally migrated code needs to updated to ES6. Also curious if we validated whether all of this code is actually needed to generate the RN theme
'!<rootDir>/src/theme/createTheme/resolveObject.ts', | ||
'!<rootDir>/src/utils/references.ts', | ||
'!<rootDir>/src/utils/groupMessages.ts', |
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.
Why would we ignore testing this code? By migrating it in to our code base we are now liable for it
const PROPERTY_REFERENCE_WARNINGS = | ||
GroupMessages.GROUP.PropertyReferenceWarnings; | ||
|
||
let current_context = []; // To maintain the context to be able to test for circular definitions |
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.
Singletons scare me, does this need to be one? Same comment for all module scoped data structures
Description of changes
ui
packageui
and consume in react native packageIssue #, if available
Description of how you validated changes
Checklist
yarn test
passes and tests are updated/addeddocs
,e2e
,examples
, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.