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

refactor: remove unused theme-proxy functionality in makeStyles() #21274

Merged
merged 2 commits into from
Jan 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "refactor: remove unused theme-proxy functionality",
"packageName": "@fluentui/babel-make-styles",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "refactor: remove unused theme-proxy functionality",
"packageName": "@fluentui/make-styles",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ describe('expressionTpl', () => {
it('returns an expression based on a template', () => {
const expression = Babel.types.identifier('foo');

const result = expressionTpl({ expression, wrapName: 'wrap', themeVariableName: 'theme' });
const result = expressionTpl({ expression, wrapName: 'wrap' });
const resultCode = generator(result).code;

expect(Babel.types.isCallExpression(result)).toBe(true);
expect(resultCode).toMatchInlineSnapshot(`"wrap(() => typeof foo === 'function' ? foo(theme) : foo)"`);
expect(resultCode).toMatchInlineSnapshot(`"wrap(() => foo)"`);
});
});
58 changes: 6 additions & 52 deletions packages/babel-make-styles/src/utils/evaluatePathsInVM.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { NodePath, types as t } from '@babel/core';
import { Scope } from '@babel/traverse';
import * as template from '@babel/template';
import generator from '@babel/generator';
import { resolveProxyValues } from '@fluentui/make-styles';
import { Module, StrictOptions } from '@linaria/babel-preset';

import type { BabelPluginOptions } from '../types';
Expand Down Expand Up @@ -55,58 +54,24 @@ const expressionWrapperTpl = template.statement(`
};
`);

/**
* Functions, call & member expressions should be wrapped with IIFE to ensure that "theme" object will be passed
* without collisions.
*
* @example
* {
* label: foo(), // call expression
* header: typography.header, // could be an object or a function
* }
*
* Outputs following template:
* @example
* wrap(() => typeof foo === 'function' ? foo(theme) : foo)
*/
export const expressionTpl = template.expression(
`%%wrapName%%(() => typeof %%expression%% === 'function' ? %%expression%%(%%themeVariableName%%) : %%expression%%)`,
);
Comment on lines -72 to -74
Copy link
Member Author

Choose a reason for hiding this comment

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

Expressions cannot be functions anymore (#21239).

export const expressionTpl = template.expression(`%%wrapName%%(() => %%expression%%)`);
const exportsPrevalTpl = template.statement(`exports.${EVAL_EXPORT_NAME} = %%expressions%%`);

/**
* Creates a new program that includes required imports and wrappers to return resolved values.
*/
function addPreval(
path: NodePath<t.Program>,
themeVariableName: string,
lazyDeps: Array<t.Expression | t.SpreadElement>,
): t.Program {
// Constant __mkPreval with all dependencies
function addPreval(path: NodePath<t.Program>, lazyDeps: Array<t.Expression | t.SpreadElement>): t.Program {
// Constant for "__mkPreval" with all dependencies
const wrapName = findFreeName(path.scope, '_wrap');
const proxyImportName = path.scope.generateUid('createCSSVariablesProxy');

const programNode = path.node;

return t.program(
// Temporary solution to solve "theme" dependency
[
t.importDeclaration(
[t.importSpecifier(t.identifier(proxyImportName), t.identifier('createCSSVariablesProxy'))],
t.stringLiteral('@fluentui/make-styles'),
),

t.variableDeclaration('const', [
t.variableDeclarator(t.identifier(themeVariableName), t.callExpression(t.identifier(proxyImportName), [])),
]),
Comment on lines -94 to -101
Copy link
Member Author

Choose a reason for hiding this comment

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

As we don't have builtin theming (#21239) we don't need to import these anymore.


...programNode.body,

expressionWrapperTpl({ wrapName }),
exportsPrevalTpl({
expressions: t.arrayExpression(
lazyDeps.map(expression => expressionTpl({ expression, wrapName, themeVariableName })),
),
expressions: t.arrayExpression(lazyDeps.map(expression => expressionTpl({ expression, wrapName }))),
}),
],
programNode.directives,
Expand All @@ -124,8 +89,6 @@ export function evaluatePathsInVM(
nodePaths: NodePath<t.Expression | t.SpreadElement>[],
pluginOptions: Required<BabelPluginOptions>,
): void {
const themeVariableName = program.scope.generateUid('theme');

const pathsToEvaluate = nodePaths.map(nodePath => {
// spreads ("...fooBar") can't be executed directly, so they are wrapped with an object ("{...fooBar}")
if (nodePath.isSpreadElement()) {
Expand All @@ -136,23 +99,14 @@ export function evaluatePathsInVM(
});

// Linaria also performs hoisting of identifiers, we don't need this as all makeStyles() calls should be top level
const modifiedProgram = addPreval(program, themeVariableName, pathsToEvaluate);
const modifiedProgram = addPreval(program, pathsToEvaluate);

const { code } = generator(modifiedProgram);
const results = evaluate(code, filename, pluginOptions);

for (let i = 0; i < nodePaths.length; i++) {
const nodePath = nodePaths[i];

// 👇 we should resolve proxy values (they are defined as functions) before creating AST from an object with styles
const result = resolveProxyValues(results[i]);

// 💡 spreads can't replace itself, we should replace it with with properties
if (nodePath.isSpreadElement()) {
nodePath.replaceWithMultiple((astify(result) as t.ObjectExpression).properties);
continue;
}
Comment on lines -150 to -154
Copy link
Member Author

Choose a reason for hiding this comment

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

We evaluate objects now (#21144), we can't meet there a spread.


nodePath.replaceWith(astify(result));
nodePath.replaceWith(astify(results[i]));
}
}
10 changes: 0 additions & 10 deletions packages/make-styles/etc/make-styles.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,6 @@ import type { OverflowProperty } from 'csstype';
// @internal
export function __styles<Slots extends string>(classesMapBySlot: CSSClassesMapBySlot<Slots>, cssRules: CSSRulesByBucket): (options: Pick<MakeStylesOptions, 'dir' | 'renderer'>) => Record<Slots, string>;

// Warning: (ae-internal-missing-underscore) The name "createCSSVariablesProxy" should be prefixed with an underscore because the declaration is marked as @internal
//
// @internal (undocumented)
export function createCSSVariablesProxy(prefix?: string): unknown;

// @public
export function createDOMRenderer(target?: Document | undefined, options?: CreateDOMRendererOptions): MakeStylesRenderer;

Expand Down Expand Up @@ -129,11 +124,6 @@ export function mergeClasses(...classNames: (string | false | undefined)[]): str
// @public
export function rehydrateRendererCache(renderer: MakeStylesRenderer, target?: Document | undefined): void;

// Warning: (ae-internal-missing-underscore) The name "resolveProxyValues" should be prefixed with an underscore because the declaration is marked as @internal
//
// @internal (undocumented)
export function resolveProxyValues<T>(value: T): T;

// Warning: (ae-internal-missing-underscore) The name "resolveStyleRules" should be prefixed with an underscore because the declaration is marked as @internal
//
// @internal
Expand Down
1 change: 0 additions & 1 deletion packages/make-styles/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ export { makeStyles } from './makeStyles';
export { resolveStyleRulesForSlots } from './resolveStyleRulesForSlots';

// Private exports, are used by build time transforms
export { createCSSVariablesProxy, resolveProxyValues } from './runtime/createCSSVariablesProxy';
export { resolveStyleRules } from './runtime/resolveStyleRules';
export { __styles } from './__styles';

Expand Down

This file was deleted.

53 changes: 0 additions & 53 deletions packages/make-styles/src/runtime/createCSSVariablesProxy.ts

This file was deleted.