-
Notifications
You must be signed in to change notification settings - Fork 313
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 #810
base: main
Are you sure you want to change the base?
Conversation
Hey @nmn, this is my initial implementation for the issue. Let me know if there are any additional test cases or improvements you'd like me to include. I’m happy to iterate further based on your feedback |
We don't want to add a new |
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.
Great start! Please fix the failing tests and address the comments.
Hey @nmn fixed all the changes you requested, i just couldnt pass the size test but apart from that i think its all ready, also updated the PR description and the unit tests, let me know if more changes are needed :) |
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.
Nearly there. I'll do some more testing before I merge, but I don't see any issues in the code review.
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.
Changes look good in general. I will test a bit more before merging.
I did some local testing and there is an issue with path normalization which makes this PR not quite work as expected. There are two issues:
This means that even though all the logic to discover the configurations is working correctly, it doesn't work in practice. You can remove the |
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.
Please fix the path resolutions.
if (tsconfig.compilerOptions?.paths) { | ||
tsconfigAliases = Object.fromEntries( | ||
Object.entries(tsconfig.compilerOptions.paths).map( | ||
([key, value]) => [ | ||
key.replace(/\/\*$/, ''), | ||
Array.isArray(value) | ||
? value.map((p) => | ||
this.normalizePath( | ||
path.join(baseUrl, p.replace(/\/\*$/, '')), | ||
), | ||
) | ||
: [ | ||
this.normalizePath( | ||
path.join(baseUrl, value.replace(/\/\*$/, '')), | ||
), | ||
], | ||
], | ||
), | ||
); | ||
} |
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.
Making this change fixed the problem for tsconfig.json
files
Similar fixes might be needed for the other config files as well.
if (tsconfig.compilerOptions?.paths) { | |
tsconfigAliases = Object.fromEntries( | |
Object.entries(tsconfig.compilerOptions.paths).map( | |
([key, value]) => [ | |
key.replace(/\/\*$/, ''), | |
Array.isArray(value) | |
? value.map((p) => | |
this.normalizePath( | |
path.join(baseUrl, p.replace(/\/\*$/, '')), | |
), | |
) | |
: [ | |
this.normalizePath( | |
path.join(baseUrl, value.replace(/\/\*$/, '')), | |
), | |
], | |
], | |
), | |
); | |
} | |
if (tsconfig.compilerOptions?.paths) { | |
tsconfigAliases = Object.fromEntries( | |
Object.entries(tsconfig.compilerOptions.paths).map( | |
([key, value]) => [ | |
key, | |
(Array.isArray(value) ? value : [value]).map((p) => { | |
const endsWithStar = p.endsWith('/*'); | |
let basePath = p.replace(/\/\*$/, ''); | |
if (basePath.startsWith('.')) { | |
console.log( | |
'trying to resolve path', | |
projectDir, | |
basePath, | |
path.resolve(projectDir, basePath), | |
); | |
basePath = path.resolve(projectDir, basePath); | |
} | |
if (endsWithStar) { | |
basePath = basePath.endsWith('/') | |
? basePath + '*' | |
: basePath + '/*'; | |
} | |
return basePath; | |
}), | |
], | |
), | |
); | |
} |
[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
, anddeno.json
. It ensures normalized paths for cross-platform compatibility and maintains backward compatibility with existing manual configurations.Implementation Details
package.json
for Node.js native subpath imports (aliases with#
prefix).tsconfig.json
forcompilerOptions.paths
and resolves paths relative tobaseUrl
.deno.json
for theimports
field.Manual >
package.json
>tsconfig.json
>deno.json
.findProjectRoot
withgetPackageNameAndPath
for project root discovery.Test Coverage
The following scenarios are tested:
1. Alias Discovery:
package.json
(subpath imports).tsconfig.json
paths.deno.json
imports.2. Configuration Merging:
3. Manual Overrides:
4. Error Handling:
Code Changes
1.
state-manager.js
(Alias Discovery)loadAliases
:stylex.aliases
support.getPackageNameAndPath
.deno.json
imports.2.
stylex-transform-alias-config-test.js
deno.json
imports.Potential Future Improvements
Enhanced Test Cases:
Additional Features:
Performance Optimizations:
Pre-flight Checklist
Feedback
Feedback is welcome, particularly for: