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.

Sign up for GitHub

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

Jump to bottom

feat: Create utils package to share logic across runtime and bundler #345

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

feat: Create utils package to share logic across runtime and bundler #345

wants to merge 7 commits into from

Conversation

brijeshb42
Copy link
Contributor

@brijeshb42 brijeshb42 commented Dec 9, 2024 •
edited
Loading

…packages

For reference, here's the next PR that uses this package.

This PR has the following utils -

  1. Pigment config base type (to be used across bundlers alongwith bundler specific keys)
  2. Processing of $$value keys in the css object to convert to css variables ---value
  3. Processing of variants, compoundVariants and defaultVariants keys
  4. valueToLiteral - Copied over from @pigment-css/react to convert JS values to equivalent AST.
  5. evaluateClassNameArg - Evaluates JS string to get the actual JS value.

Also, fixes tsconfig to used the latest config from core and updated the script to actually run typecheck in the CI which was not happening right now.

@brijeshb42 brijeshb42 added the @pigment-css/utils Specific to @pigment-css/utils package label Dec 9, 2024
@brijeshb42 brijeshb42 added this to the Road to v1 milestone Dec 9, 2024
@zannager zannager requested a review from mnajdova December 10, 2024 15:32
@brijeshb42 brijeshb42 force-pushed the v1/utils-package branch 6 times, most recently from e1c0ebc to 903cc78 Compare December 11, 2024 11:12
mnajdova
mnajdova reviewed Dec 20, 2024
View reviewed changes
packages/pigment-css-nextjs-plugin/tsconfig.json
@@ -1,8 +1,7 @@
{
"extends": "../../tsconfig.json",
"compilerOptions": {
"resolveJsonModule": true,
"target": "ES2015"
"skipLibCheck": true
Copy link
Member

Choose a reason for hiding this comment

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

Why was this required?

Copy link
Contributor Author

@brijeshb42 brijeshb42 Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Without it, it was typechecking for stuff inside unplugin package that does not affect our package.

../../node_modules/.pnpm/unplugin@1.15.0_webpack-sources@3.2.3/node_modules/unplugin/dist/index.d.mts(1,31): error TS2307: Cannot find module '@farmfe/core' or its corresponding type declarations.
      ../../node_modules/.pnpm/unplugin@1.15.0_webpack-sources@3.2.3/node_modules/unplugin/dist/index.d.mts(2,46): error TS2307: Cannot find module '@farmfe/core' or its corresponding type declarations.
      ../../node_modules/.pnpm/unplugin@1.15.0_webpack-sources@3.2.3/node_modules/unplugin/dist/index.d.mts(3,47): error TS2307: Cannot find module '@rspack/core/dist/config/zod' or its corresponding type declarations.
      ../../node_modules/.pnpm/unplugin@1.15.0_webpack-sources@3.2.3/node_modules/unplugin/dist/index.d.mts(7,45): error TS2307: Cannot find module 'rolldown/dist/types/plugin' or its corresponding type declarations.
      ../../node_modules/.pnpm/unplugin@1.15.0_webpack-sources@3.2.3/node_modules/unplugin/dist/index.d.mts(14,126): error TS2307: Cannot find module '@rspack/core' or its corresponding type declarations.
      ../../node_modules/.pnpm/unplugin@1.15.0_webpack-sources@3.2.3/node_modules/unplugin/dist/index.d.mts(15,66): error TS2307: Cannot find module '@rspack/core' or its corresponding type declarations.
      ../../node_modules/.pnpm/unplugin@1.15.0_webpack-sources@3.2.3/node_modules/unplugin/dist/index.d.mts(19,36): error TS2307: Cannot find module 'rolldown' or its corresponding type declarations.
      ../../node_modules/.pnpm/unplugin@1.15.0_webpack-sources@3.2.3/node_modules/unplugin/dist/index.d.mts(20,42): error TS2307: Cannot find module 'rolldown' or its corresponding type declarations.
       ELIFECYCLE  Command failed with exit code 2.

This check can be safely ignored since at the moment we don't care about farm/rolldown/rspack etc.

packages/pigment-css-theme/package.json Outdated Show resolved Hide resolved
packages/pigment-css-utils/package.json Outdated Show resolved Hide resolved
packages/pigment-css-utils/src/utils/parseExpression.ts Outdated Show resolved Hide resolved
packages/pigment-css-utils/src/utils/processStyle.ts
>;
defaultVariants?: Record<string, string | number>;
};
type BaseStyleObject = ExtendedStyleObj & Record<string, string | object>;
Copy link
Member

Choose a reason for hiding this comment

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

Don't we miss here the CSS template string type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Template string doesn't need any pre-processing as we are not extending the css string syntax, only the object syntax. Template strings are just string concatenation with some js values in-between.

packages/pigment-css-utils/tests/utils/processStyle.test.ts
},
},
variables: {
'--var-1': [style[`.cls1`].color, 0],
Copy link
Member

Choose a reason for hiding this comment

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

Where would this style[`.cls1`].color be defined?

Copy link
Contributor Author

@brijeshb42 brijeshb42 Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm just using the original style object passed to the processStyle function to compare the function equality check by comparing the references.

packages/pigment-css-utils/tests/utils/processStyle.test.ts
Comment on lines +13 to +14
$$hello: 'world',
$hello1: 'world',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$$hello: 'world',
$hello1: 'world',
$$hello: 'world',
$hello1: 'world',

What's the difference betweem $ and $$?

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 convention here is that $$ is more like private variables an it'll have 3 dashes ---var and doesn't need to be exposed to users for customization. $ is public variables that can be exposed through documentation to customize the style and it'll have 2 dashes --var

@brijeshb42 brijeshb42 force-pushed the v1/utils-package branch 4 times, most recently from a4b05a1 to 8dcf767 Compare December 25, 2024 05:30
@brijeshb42 brijeshb42 requested a review from mnajdova December 25, 2024 06:29
@brijeshb42 brijeshb42 force-pushed the v1/utils-package branch 3 times, most recently from 35d32d4 to 7508d32 Compare December 27, 2024 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnajdova mnajdova Awaiting requested review from mnajdova

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
@pigment-css/utils Specific to @pigment-css/utils package
Projects
None yet
Milestone
Road to v1
Development

Successfully merging this pull request may close these issues.
2 participants
@brijeshb42 @mnajdova