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: Allow stylex.create nested within TS namespaces #841

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

Conversation

nonzzz
Copy link
Contributor

@nonzzz nonzzz commented Jan 7, 2025

What changed / motivation ?

Now current work is WIP, but i need to clarify some situations.

Linked PR/Issues

#829

Additional Context

Q: Should we add a manipulateOptions for babel plugin to infer the coming file is typescript or others?
Q: Should the export expression check of Theme's API be cancelled? If embedded in typescript namespace.

Pre-flight checklist

@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 7, 2025
@necolas
Copy link
Contributor

necolas commented Jan 7, 2025

Why can't the TS source code be converted to JS before babel/stylex runs?

Copy link
Contributor

@nmn nmn left a 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)
Copy link
Contributor

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!

Comment on lines 39 to 58
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;
}

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

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.

4 participants