-
Notifications
You must be signed in to change notification settings - Fork 356
Feat: Automatically Discover StyleX Aliases from Configuration Files #810
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
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 |
nmn
left a comment
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 :) |
nmn
left a comment
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.
nmn
left a comment
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 |
nmn
left a comment
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; | |
| }), | |
| ], | |
| ), | |
| ); | |
| } |
|
Closing in favor of #856 |
[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.jsonfor Node.js native subpath imports (aliases with#prefix).tsconfig.jsonforcompilerOptions.pathsand resolves paths relative tobaseUrl.deno.jsonfor theimportsfield.Manual >
package.json>tsconfig.json>deno.json.findProjectRootwithgetPackageNameAndPathfor project root discovery.Test Coverage
The following scenarios are tested:
1. Alias Discovery:
package.json(subpath imports).tsconfig.jsonpaths.deno.jsonimports.2. Configuration Merging:
3. Manual Overrides:
4. Error Handling:
Code Changes
1.
state-manager.js(Alias Discovery)loadAliases:stylex.aliasessupport.getPackageNameAndPath.deno.jsonimports.2.
stylex-transform-alias-config-test.jsdeno.jsonimports.Potential Future Improvements
Enhanced Test Cases:
Additional Features:
Performance Optimizations:
Pre-flight Checklist
Feedback
Feedback is welcome, particularly for: