-
Notifications
You must be signed in to change notification settings - Fork 318
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: Allow stylex.create
nested within TS namespaces
#841
base: main
Are you sure you want to change the base?
Conversation
Why can't the TS source code be converted to JS before babel/stylex runs? |
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.
Could you make one quick change to the tests and then I can merge
@@ -343,7 +343,8 @@ function validateStyleXCreate(path: NodePath<t.CallExpression>) { | |||
const nearestStatement = findNearestStatementAncestor(path); | |||
if ( | |||
!pathUtils.isProgram(nearestStatement.parentPath) && | |||
!pathUtils.isExportNamedDeclaration(nearestStatement.parentPath) | |||
!pathUtils.isExportNamedDeclaration(nearestStatement.parentPath) && | |||
!pathUtils.isTSModuleBlock(nearestStatement.parentPath) |
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.
Yup. This should fix the issue for now!
function transformTypescript(source, opts = {}) { | ||
return transformSync(source, { | ||
filename: opts.filename, | ||
parserOpts: { | ||
plugins: ['typescript'], | ||
}, | ||
babelrc: false, | ||
plugins: [ | ||
[ | ||
stylexPlugin, | ||
{ | ||
runtimeInjection: true, | ||
unstable_moduleResolution: { type: 'haste' }, | ||
...opts, | ||
}, | ||
], | ||
], | ||
}).code; | ||
} | ||
|
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.
Can we update the existing transform
function to use typescript based on opts.filename
instead?
That way, we won't need two mostly duplicate functions.
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.
BTW, I think that's not enough ( Maybe shouldn't judge whether it is a tsBlock
) The final conversion result of each compiler
import stylex from "@stylexjs/stylex";
export var X;
((X2) => {
X2.styles = stylex.create({
t: {
color: "red"
}
});
})(X || (X = {}));
If we want to support namespace, then we should also support this.
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.
If this is the output then this is not yet going to be feasible.
We can hoist up the generated objects to the top level of the module and replace the original stylex.create
with a reference to that object. That way we can allow defining stylex.create
anywhere you want.
What changed / motivation ?
Now current work is WIP, but i need to clarify some situations.
Linked PR/Issues
#829
Additional Context
Pre-flight checklist
Contribution Guidelines