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: Remove style-dictionary dependency from React Native package #5848

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

esauerbo
Copy link
Contributor

Description of changes

  • Copy style dictionary utils/functions to ui package
  • Export from ui and consume in react native package

Issue #, if available

Description of how you validated changes

Checklist

  • Have read the Pull Request Guidelines
  • PR description included
  • yarn test passes and tests are updated/added
  • PR title and commit messages follow conventional commit syntax
  • If this change should result in a version bump, changeset added (This can be done after creating the PR.) This does not apply to changes made to docs, 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.

@esauerbo esauerbo requested a review from a team as a code owner September 27, 2024 20:20
Copy link

changeset-bot bot commented Sep 27, 2024

🦋 Changeset detected

Latest commit: 0eae19c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
Name Type
@aws-amplify/ui-react-native Patch
@aws-amplify/ui Patch
@aws-amplify/ui-react-native-auth Patch
@aws-amplify/ui-react-auth Patch
@aws-amplify/ui-react-core-auth Patch
@aws-amplify/ui-react-core-notifications Patch
@aws-amplify/ui-react-core Patch
@aws-amplify/ui-react-liveness Patch
@aws-amplify/ui-react-notifications Patch
@aws-amplify/ui-react-storage Patch
@aws-amplify/ui-react Patch
@aws-amplify/ui-vue Patch
@aws-amplify/ui-angular Patch
@aws-amplify/ui-react-geo Patch

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',
Copy link
Contributor Author

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from style-dictionary@3.9.1 which is specified in the ui package.json

};
let updated_object, regex, options;

export function resolveObject<T>(object: Record<string, any>): T {
Copy link
Contributor Author

@esauerbo esauerbo Sep 27, 2024

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from style-dictionary@3.9.1 which is specified in the ui package.json, no changes made

@@ -0,0 +1,112 @@
import { has } from './utils';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from style-dictionary@3.9.1. Only change is using our util has in favor of hasOwnProperty

@@ -290,3 +290,23 @@ export function splitObject(
});
return [left, right] as const;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from style-dictionary@3.9.1, did not change anything

Copy link
Contributor

@jordanvn jordanvn left a 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

Copy link
Contributor

@calebpollman calebpollman left a 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

Comment on lines +12 to +14
'!<rootDir>/src/theme/createTheme/resolveObject.ts',
'!<rootDir>/src/utils/references.ts',
'!<rootDir>/src/utils/groupMessages.ts',
Copy link
Contributor

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
Copy link
Contributor

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

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.

4 participants