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

Feat: Automatically Discover StyleX Aliases from Configuration Files (Rebased PR of #810) #856

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

Conversation

nmn
Copy link
Contributor

@nmn nmn commented Jan 13, 2025

What changed / motivation ?

This a rebased implementation of #810 with bug fixes.


[Feature] Automatically Discover StyleX Aliases from Configuration Files

Linked Issues

Fixes #765


Overview

This PR enhances the StyleX Babel plugin to automatically discover alias configurations from Node.js native imports (package.json), tsconfig.json, and deno.json. It ensures normalized paths for cross-platform compatibility and maintains backward compatibility with existing manual configurations.


Implementation Details

  • JSON5 Parser: Handles configuration files with comments or trailing commas.
  • Alias Discovery:
    • Parses package.json for Node.js native subpath imports (aliases with # prefix).
    • Parses tsconfig.json for compilerOptions.paths and resolves paths relative to baseUrl.
    • Parses deno.json for the imports field.
  • Merge Configurations:
    • Combines configurations from all sources:
      Manual > package.json > tsconfig.json > deno.json.
  • Code Reuse:
    • Replaced redundant findProjectRoot with getPackageNameAndPath for project root discovery.
  • Normalized Paths:
    • Standardizes path separators for cross-platform compatibility.
  • Error Handling:
    • Gracefully handles missing or malformed configuration files.

Test Coverage

The following scenarios are tested:

1. Alias Discovery:

  • Node.js native imports from package.json (subpath imports).
  • Aliases from tsconfig.json paths.
  • Aliases from deno.json imports.

2. Configuration Merging:

  • Merges aliases from all sources with correct priority.

3. Manual Overrides:

  • Ensures manual alias configurations take precedence over discovered aliases.

4. Error Handling:

  • Handles missing or invalid configuration files gracefully.

Code Changes

1. state-manager.js (Alias Discovery)

  • Updated Functionality:
    • loadAliases:
      • Now supports Node.js native imports and deno.json.
      • Removed custom stylex.aliases support.
      • Simplified project root discovery using getPackageNameAndPath.
  • Added support for deno.json imports.

2. stylex-transform-alias-config-test.js

  • Updated Unit Tests:
    • Tests for Node.js native imports discovery.
    • Tests for deno.json imports.
    • Tests for merged configuration from all sources.
    • Tests for proper handling of invalid configurations.

Potential Future Improvements

  1. Enhanced Test Cases:

    • Complex alias patterns with wildcards.
    • Nested directory structures.
    • Edge cases in path resolution.
  2. Additional Features:

    • Support for other configuration sources like Webpack config.
    • More granular control over alias resolution priorities.
  3. Performance Optimizations:

    • Caching of discovered configurations.
    • Smarter file system traversal.

Pre-flight Checklist

  • I have read the Contribution Guidelines.
  • Performed a self-review of my code.
  • Added tests for the new functionality.
  • Verified cross-platform compatibility.
  • Ensured backward compatibility with existing configurations.

Feedback

Feedback is welcome, particularly for:

  • Additional test cases to consider.
  • Improvements to performance or alias resolution edge cases.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 13, 2025
Copy link

github-actions bot commented Jan 13, 2025

workflow: benchmarks/size

Comparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.

@stylexjs/[email protected] size:compare
./size-compare.js /tmp/tmp.55rA5E13tE /tmp/tmp.c1GNyNSMA5

Results Base Patch Ratio
stylex/lib/stylex.js
· compressed 950 950 1.00
· minified 3,051 3,051 1.00
stylex/lib/StyleXSheet.js
· compressed 1,266 1,266 1.00
· minified 3,776 3,776 1.00
rollup-example/.build/bundle.js
· compressed 567,170 567,170 1.00
· minified 10,232,457 10,232,457 1.00
rollup-example/.build/stylex.css
· compressed 100,628 100,628 1.00
· minified 755,718 755,718 1.00

@nmn nmn marked this pull request as draft January 17, 2025 22:53
@nmn nmn force-pushed the feat/auto-aliases branch 2 times, most recently from a4654f6 to 105df51 Compare February 10, 2025 04:39
@nmn nmn changed the title Rebased PR of #810 Feat: Automatically Discover StyleX Aliases from Configuration Files (Rebased PR of #810) Feb 10, 2025
@nmn nmn marked this pull request as ready for review February 10, 2025 04:41
@nmn nmn force-pushed the feat/auto-aliases branch from 105df51 to 639a876 Compare February 10, 2025 04:44
@nmn nmn requested review from mellyeliu and necolas February 10, 2025 05:10
const [_packageName, projectDir] = pkgInfo;

const resolveAliasPaths = (value: string | $ReadOnlyArray<string>) =>
(Array.isArray(value) ? value : [value]).map((p) => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: we should consider moving some of this logic to a separate files utils, there's some path functions in the cache.js utils that I also want to extract

...denoAliases,
...tsconfigAliases,
...packageAliases,
...(manualAliases || {}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need manualAliases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to remove it in the same commit, but we could probably remove them in the future once we are sure this feature is working for the wide variety of project types.

We could deprecate it in the next release and delete it entirely after.

p0nch000 and others added 3 commits February 12, 2025 20:39
Fixed flow and prettier tests failing

Correct install of json5 dependency

fix: Resolve alias paths to absolute paths
import os from 'os';
import StateManager from '../src/utils/state-manager';

describe('StyleX Alias Configuration', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please write tests against the public API. If we refactor the internals, tests like this would likely need to be rewritten. And this test doesn't actually validate that real code is correctly processed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. Will need to simulate a filesystem or similar to make this happen, but should be doable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[babel-plugin] Automatically pick up alias configuration from package.json or tsconfig
5 participants