[NoQA] feat: react-compiler healthcheck #44460

kirillzyusko · 2024-06-26T13:54:32Z

Details

Gives a list of components that can not be optimized with a reason why:

Also changes from this PR:

bumped linter, so now it throws errors if it detects violations (looks like before it wasn't working) + removed unused patch;
bumped babel plugin (react-compiler) (and it looks like now it can optimize 900/1200 files - before it was 600/1200);

Most common mistakes

Agenda:

🔴 - has one of next severities: InvalidJS/InvalidReact/InvalidConfig/CannotPreserveMemoization/Invariant
⚠️ - has ToDo severity

Warning

If error has ToDo severity, then most likely this will be handled in future compiler versions and the component will be optimized (but right now such files are not optimized).

🔴 Ignored eslint rule

useEffect(() => {
    subscribeToNavigationShortcuts();

    // If we're coming from Plaid OAuth flow then we need to reuse the existing plaidLinkToken
    if (isAuthenticatedWithPlaid()) {
        return unsubscribeToNavigationShortcuts;
    }
    BankAccounts.openPlaidBankLogin(allowDebit, bankAccountID);
    return unsubscribeToNavigationShortcuts;

    // disabling this rule, as we want this to run only on the first render
    // eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

Failed to compile src/components/AddPlaidBankAccount.tsx:156:8. Reason: React Compiler has skipped optimizing this component because one or more React ESLint rules were disabled. React Compiler only works when your components follow all the rules of React, disabling them may result in unexpected or incorrect behavior

🔴 Modifying `SharedValue` (reactwg/react-compiler#14)

React.useEffect(() => {
    if (shouldDim) {
        opacity.value = withTiming(dimmingValue, {duration: 50});
    } else {
        opacity.value = withTiming(1, {duration: 50});
    }
}, [shouldDim, dimmingValue, opacity]);

Mutating a value returned from a function whose return value should not be mutated

🔴 Modifying `ref`?

const enterFullScreenMode = useCallback(() => {
  isFullScreenRef.current = true;

Mutating a value returned from a function whose return value should not be mutated

🔴 `useNativeDriver` is treated as a hook 😅

<Animatable.View
    onAnimationEnd={() => {
        if (!onAnimationEnd) {
            return;
        }
        onAnimationEnd();
    }}
    duration={CONST.ANIMATED_TRANSITION}
    animation={animationStyle}
    useNativeDriver={useNativeDriver}
    style={style}
>

Hooks may not be referenced as normal values, they must be called. See https://react.dev/reference/rules/react-calls-components-and-hooks#never-pass-around-hooks-as-regular-values

🔴 Props mutation (even if it's ref/SharedValue)

function DisplayNamesTooltipItem({
    index = 0,
    getTooltipShiftX = () => 0,
    accountID = 0,
    avatar = '',
    login = '',
    displayName = '',
    textStyles = [],
    childRefs = {current: []},
}: DisplayNamesTooltipItemProps) {
// ...
ref={(el) => {
    // eslint-disable-next-line no-param-reassign
    childRefs.current[index] = el; // <- triiger
}}

// or...

const usePanGesture = ({
    canvasSize,
    contentSize,
    zoomScale,
    totalScale,
    offsetX,
) => {
// later on
offsetX.value = withDecay({
}

Mutating component props or hook arguments is not allowed. Consider using a local variable instead

🔴 dependency list should be an array literal

function useMergeRefs(...args: Array<MutableRefObject<FlatList> | ForwardedRef<FlatList> | null>) {
    return useMemo(
        () => mergeRefs(...args),
        // eslint-disable-next-line
        [...args],
    );
}

Expected the dependency list for useMemo to be an array literal

🔴 mutating external variables

let activeRouteName = '';
function FocusTrapForScreen({children}: FocusTrapProps) {
    useFocusEffect(
        useCallback(() => {
            activeRouteName = route.name; // <-- trigger
        }, [route]),
    );
}

Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)

🔴 Writing to a variable defined outside

const isScrollEnabled = attachmentCarouselPagerContext === null ? undefined : attachmentCarouselPagerContext.isScrollEnabled;

const Pan = Gesture.Pan()
    .manualActivation(true)
    .onTouchesMove((evt) => {
        if (offsetX.value !== 0 && offsetY.value !== 0 && isScrollEnabled) {
            const translateX = Math.abs(evt.allTouches[0].absoluteX - offsetX.value);
            const translateY = Math.abs(evt.allTouches[0].absoluteY - offsetY.value);
            const allowEnablingScroll = !isPanGestureActive.value || isScrollEnabled.value;

            // if the value of X is greater than Y and the pdf is not zoomed in,
            // enable  the pager scroll so that the user
            // can swipe to the next attachment otherwise disable it.
            if (translateX > translateY && translateX > SCROLL_THRESHOLD && scale.value === 1 && allowEnablingScroll) {
                isScrollEnabled.value = true;

Writing to a variable defined outside a component or hook is not allowed. Consider using an effect

⚠️ Expected Identifier, got NumericLiteral key in ObjectExpression

<DotIndicatorMessage
    style={[styles.mt6]}
    // eslint-disable-next-line @typescript-eslint/naming-convention
    messages={{0: translate(errorData.validationError, errorData.phraseParam as never)}}
    type="error"
/>

(BuildHIR::lowerExpression) Expected Identifier, got NumericLiteral key in ObjectExpression

🔴 complex manual memoization

const selectedOptions = useMemo(() => {
    if (!selectedCategory) {
        return [];
    }

    return [
        {
            name: selectedCategory,
            accountID: undefined,
            isSelected: true,
        },
    ];
}, [selectedCategory]);

React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value may be mutated later, which could cause the value to change unexpectedly

🔴 Inferred dependencies did not match manually specified dependencies

const selectedOptionKey = useMemo(() => (sections?.[0]?.data ?? []).filter((category) => category.searchText === selectedCategory)[0]?.keyForList, [sections, selectedCategory]);

React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected

⚠️ Expected Identifier, got MemberExpression key in ObjectExpression

<Text
    style={combinedTitleTextStyle}
    numberOfLines={numberOfLinesTitle || undefined}
    dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: interactive && disabled}}
>
    {renderTitleContent()}
</Text>

BuildHIR::lowerExpression) Expected Identifier, got MemberExpression key in ObjectExpression

🔴 Invalid nesting in program blocks or scopes

/**
 * TextBlock component splits a given text into individual words and displays
 * each word within a Text component.
 */
import React, {memo, useMemo} from 'react';
import type {StyleProp, TextStyle} from 'react-native';
import Text from './Text';

type TextBlockProps = {
    /** The color of the text */
    color?: string;

    /** Styles to apply to each text word */
    textStyles?: StyleProp<TextStyle>;

    /** The full text to be split into words */
    text: string;
};

function TextBlock({color, textStyles, text}: TextBlockProps) {
    const words = useMemo(() => text.match(/(\S+\s*)/g) ?? [], [text]);

    return (
        <>
            {words.map((word) => (
                <Text
                    color={color}
                    style={textStyles}
                >
                    {word}
                </Text>
            ))}
        </>
    );
}

TextBlock.displayName = 'TextBlock';

export default memo(TextBlock);

Invalid nesting in program blocks or scopes

Warning

There is no specific lines, it just complains about entire file.

⚠️ Expression type `ArrowFunctionExpression` cannot be safely reordered

function TextLink({href, onPress, children, style, onMouseDown = (event) => event.preventDefault(), ...rest}: TextLinkProps, ref: ForwardedRef<RNText>) {

Reason: (BuildHIR::node.lowerReorderableExpression) Expression type ArrowFunctionExpression cannot be safely reordered

⚠️ Expected Identifier, got LogicalExpression key in ObjectExpression

<Context.Consumer>
  {(value) => {
      const propsToPass = {
          ...props,
          [propName ?? onyxKeyName]: transformValue ? transformValue(value, props) : value, // <-- this line
      } as TProps;

(BuildHIR::lowerExpression) Expected Identifier, got LogicalExpression key in ObjectExpression

⚠️ Handle Identifier inits in ForInStatement

return useMemo(() => {
        const permissions: UsePermissions = {};

        for (permissionKey in Permissions) { // <-- this cycle
            if (betas) {
                const checkerFunction = Permissions[permissionKey];

                permissions[permissionKey] = checkerFunction(betas, iouType);
            }
        }

        return permissions;
    }, [betas, iouType]);

(BuildHIR::lowerStatement) Handle Identifier inits in ForInStatement

⚠️ Expression type `TSAsExpression` cannot be safely reordered

function ReportAvatar({
  report = {} as Report, // <-- this line
  policies, 
  isLoadingApp = true}: ReportAvatarProps) {

(BuildHIR::node.lowerReorderableExpression) Expression type TSAsExpression cannot be safely reordered

⚠️ Handle UpdateExpression with MemberExpression argument

useEffect(() => {
        if (isDisabled) {
            return;
        }

        // Update status bar when theme changes
        updateStatusBarStyle();

        // Add navigation state listeners to update the status bar every time the route changes
        // We have to pass a count as the listener id, because "react-navigation" somehow doesn't remove listeners properly
        const listenerID = ++listenerCount.current; // <-- this line
        const listener = () => updateStatusBarStyle(listenerID);

        navigationRef.addListener('state', listener);
        return () => navigationRef.removeListener('state', listener);
    }, [isDisabled, updateStatusBarStyle]);

(BuildHIR::lowerExpression) Handle UpdateExpression with MemberExpression argument

⚠️ Expression type `CallExpression` cannot be safely reordered

function DatePicker(
    {
        containerStyles,
        defaultValue,
        disabled,
        errorText,
        inputID,
        label,
        maxDate = setYear(new Date(), CONST.CALENDAR_PICKER.MAX_YEAR), // <-- this line
        minDate = setYear(new Date(), CONST.CALENDAR_PICKER.MIN_YEAR),

(BuildHIR::node.lowerReorderableExpression) Expression type CallExpression cannot be safely reordered

⚠️ Unexpected terminal kind `optional` for logical test block

const isEditingSplitBill = session?.accountID === /*complains here*/ reportAction?.actorAccountID && TransactionUtils.areRequiredFieldsEmpty(transaction);

Unexpected terminal kind logical for logical test block

⚠️ Expression type `NewExpression` cannot be safely reordered

function CalendarPicker({
    value = new Date(), // <-- this line

Expression type NewExpression cannot be safely reordered

⚠️ Handle computed properties in ObjectPattern

const toggleCategory = (category: PolicyOption) => {
    setSelectedCategories((prev) => {
        if (prev[category.keyForList]) {
            const /*complains here*/{[category.keyForList]: omittedCategory, ...newCategories} = prev;
            return newCategories;

(BuildHIR::lowerAssignment) Handle computed properties in ObjectPattern

⚠️ Handle TSSatisfiesExpression expressions

const submitForm = useCallback(
    ({value, ...values}: FormOnyxValues<typeof ONYXKEYS.FORMS.WORKSPACE_NEW_TAX_FORM>) => {
        const taxRate = {
            ...values,
            value: getTaxValueWithPercentage(value),
            code: getNextTaxCode(values[INPUT_IDS.NAME], policy?.taxRates?.taxes),
        } satisfies TaxRate;

(BuildHIR::lowerExpression) Handle TSSatisfiesExpression expressions

⚠️ Handle tagged template with interpolations

    const validate = (values: FormOnyxValues<typeof ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM>): FormInputErrors<typeof ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM> => {
        const errors: FormInputErrors<typeof ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM> = {};
        const amountValues = getAmountValues(values);
        const outputCurrency = policy?.outputCurrency ?? CONST.CURRENCY.USD;
        const amountRegex = RegExp(String.raw`^-?\d{0,8}([${permittedDecimalSeparator}]\d{0,${CurrencyUtils.getCurrencyDecimals(outputCurrency)}})?$`, 'i'); // <-- complains here

(BuildHIR::lowerExpression) Handle tagged template with interpolations

⚠️ Unexpected terminal kind `optional` for ternary test block

const sortedLoginNames = loginNames.sort((loginName) => (loginList?.[loginName].partnerUserID === session?.email ? -1 : 1));

Unexpected terminal kind optional for ternary test block

⚠️ Expected Identifier, got TemplateLiteral key in ObjectExpression

 const updateMapping = useCallback(
        (option: {value: string}) => {
            if (option.value !== categoryName) {
                Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.XERO, CONST.XERO_CONFIG.MAPPINGS, {
                    ...(policy?.connections?.xero?.config?.mappings ?? {}),
                    ...(categoryId ? {[`${CONST.XERO_CONFIG.TRACKING_CATEGORY_PREFIX}${categoryId}`]: option.value} : {}), // <-- here
                });
            }

(BuildHIR::lowerExpression) Expected Identifier, got TemplateLiteral key in ObjectExpression

Fixed Issues

$ #44384
PROPOSAL:

Tests

N/A

Offline tests

N/A

QA Steps

N/A

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

hannojg · 2024-06-28T09:01:14Z

Looks awesome!

Two questions:

When does the health check run?
Let's make a plan on successively fixing some of the errors that can be addressed?

hannojg · 2024-06-28T09:01:50Z

Re 1. Stupid me, just seeing the script in the package.json, looks good 👍
at some point this should be added as a CI job I think

melvin-bot · 2024-06-28T09:16:49Z

@Pujan92 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

kirillzyusko · 2024-06-28T09:26:39Z

Let's make a plan on successively fixing some of the errors that can be addressed?

Definitely!

@adamgrzybowski @blazejkustra @mountiny I'm tagging you, because I'd like to discuss a further approach.

I have next open questions:

I bumped eslint for react-compiler and now it reports violations - to make this PR merged should I simply ignore all reported violations?
The current version of react-compiler definitely has some false-positives. For example when we pass useNativeDriver as prop - compiler thinks that we are passing a hook (though we pass a boolean value). when we modify SharedValue - it complains about that (I guess it thinks that SharedValue is a signal - but need to check). And some of other reports may look like a false positives. I'd like to work on it and make an investigation further and submit PRs to upstream.
We need to define a strategy of a further adoption. The current plan is that all new files we try to write without manual usage of useMemo/useCallback, right? I'm just thinking on how to make sure, that people are using react-compiler, but the files they write are actually can be compiled successfully. Ideally to have a job on CI that will check, that no new "uncompilable" files are detected. But at the early stage of adoption it can be an overkill? What do you think? I'll be glad to hear your thoughts! 😊
Expensify codebase also has some violations (for example we often don't include all necessary deps and ignore a eslint rule). react-compiler rejects to compile such files. What is the plan on how to fix that? I think it's not a big problem right now (because our manual memoization will work in these files) but would be good to have a plan on how to resolve such issues in a near future 👀

mountiny · 2024-06-28T10:09:10Z

I bumped eslint for react-compiler and now it reports violations - to make this PR merged should I simply ignore all reported violations?

Yes I think in this PR we should not be fixing the violations

I'd like to work on it and make an investigation further and submit PRs to upstream.

I think that would be great, you can do that but also if you are getting stuck, maybe just try to log the upstream issuew with your findings and do not go the rabbithole for too long

The current plan is that all new files we try to write without manual usage of useMemo/useCallback, right? I'm just thinking on how to make sure, that people are using react-compiler, but the files they write are actually can be compiled successfully. Ideally to have a job on CI that will check, that no new "uncompilable" files are detected. But at the early stage of adoption it can be an overkill? What do you think? I'll be glad to hear your thoughts! 😊

Those are good points. I think, ideally, we could create a new ESLint rule that would prevent people from adding new useMemo or useCallback while allowing them to keep using the existing ones.

I think we can wait a bit more with the CI approach; maybe at the end of next week or in two weeks, we will add a check that will verify that any new file is compatible, and it will throw an error if there is some new page that is not compatible. Then we can slowly get through the existing incompatible pages and fix them.

Expensify codebase also has some violations (for example we often don't include all necessary deps and ignore a eslint rule). react-compiler rejects to compile such files.

How many files is this roughly, do you know? We try to avoid big migrations as it takes lots of management overhead but I assume we could take a sweep at this to get it all sorted

kirillzyusko · 2024-06-28T18:04:22Z

Those are good points. I think, ideally, we could create a new ESLint rule that would prevent people from adding new useMemo or useCallback while allowing them to keep using the existing ones.

I think we can wait a bit more with the CI approach; maybe at the end of next week or in two weeks, we will add a check that will verify that any new file is compatible, and it will throw an error if there is some new page that is not compatible. Then we can slowly get through the existing incompatible pages and fix them.

Yep, I think it's a great idea! 👍

How many files is this roughly, do you know? We try to avoid big migrations as it takes lots of management overhead but I assume we could take a sweep at this to get it all sorted

I think ~300 (from 1200 total files). I'm also against big migrations, but I think for react-compiler we can follow an approach of gradual adoption - and suggested linter rules (for useMemo/useCallback) can help us to adopt everything step-by-step 👍

I think this PR is ready for review now.

mountiny · 2024-06-28T20:53:48Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

mountiny · 2024-06-28T20:55:39Z

@kirillzyusko there seems to be conflicts now

kirillzyusko · 2024-06-30T10:43:48Z

there seems to be conflicts now

@mountiny resolved them! Thank you for the reminder 🙌

blazejkustra · 2024-07-01T12:42:44Z

@adamgrzybowski @blazejkustra @mountiny I'm tagging you, because I'd like to discuss a further approach.

I think you meant to tag @adhorodyski here 😅

I bumped eslint for react-compiler and now it reports violations - to make this PR merged should I simply ignore all reported violations?

Yep, that sounds good 👍

We need to define a strategy of a further adoption. The current plan is that all new files we try to write without manual usage of useMemo/useCallback, right? I'm just thinking on how to make sure, that people are using react-compiler, but the files they write are actually can be compiled successfully. Ideally to have a job on CI that will check, that no new "uncompilable" files are detected. But at the early stage of adoption it can be an overkill? What do you think? I'll be glad to hear your thoughts! 😊

Workflow is an overkill for this imo, I think we agreed that an eslint rule that warns whenever useCallback/useMemo is imported should do the trick here. Could you list some alternative approaches to this? (other than workflow, eslint warnings)

I think, ideally, we could create a new ESLint rule that would prevent people from adding new useMemo or useCallback while allowing them to keep using the existing ones.

Is this possible to create a rule that throws only for new imports and allow the old ones?

Expensify codebase also has some violations (for example we often don't include all necessary deps and ignore a eslint rule). react-compiler rejects to compile such files. What is the plan on how to fix that? I think it's not a big problem right now (because our manual memoization will work in these files) but would be good to have a plan on how to resolve such issues in a near future 👀

How many occurrences are there? Unfortunately it's a common approach to remove some of the deps to decrease number of rerenders. That might be a big refactor with possible regressions 😢

kirillzyusko · 2024-07-01T16:47:23Z

I think you meant to tag @adhorodyski here 😅

Shame on me 🤦‍♂️ Yeah, I meant to tag @adhorodyski thank you for raising this 🙌

Workflow is an overkill for this imo, I think we agreed that an eslint rule that warns whenever useCallback/useMemo is imported should do the trick here. Could you list some alternative approaches to this? (other than workflow, eslint warnings)

I think the ideal approach is to:

enable react-compiler eslint (done in this PR)
to write our own rule (if we see useCallback/useMemo usage then it's fine to continue for this file to use that), otherwise if it's first occurrence, then throw eslint error

Is this possible to create a rule that throws only for new imports and allow the old ones?

I think it should be possible, yes.

How many occurrences are there? Unfortunately it's a common approach to remove some of the deps to decrease number of rerenders. That might be a big refactor with possible regressions 😢

In this PR where I disabled eslint react-compiler/react-compiler - all these files can not be compiled because of that. So it's around 150 files.

The other thing is that react-compiler right now reports a lot of false-positives. For example we often execute useEffect(() => {}, []) to be fired on mount and intentionally ignore elint rule - but react-compiler complains about that (but open question is why it's complaining on useEffect if it's not optimizing effects?).

Anyway, I think from these 150 files ~30% of files will be fixed later in the compiler itself. And other files we'll have to fix at some point of time 👀

blazejkustra · 2024-07-02T14:07:42Z

Is this possible to create a rule that throws only for new imports and allow the old ones?

I think it should be possible, yes.

Based on my understanding, ESLint doesn't inherently support distinct rules for 'new' vs 'old' imports, given that it evaluates files as they exist at a particular point in time without knowledge of their history. Can you confirm this? Or are there any workarounds to achieve a similar effect?

We need to define a strategy of a further adoption.

I agree, how about we move over to slack with this discussion? Let's showcase all possible approaches so that we have a clear way forward when it comes to adoption.

kirillzyusko · 2024-07-02T15:40:11Z

Based on my understanding, ESLint doesn't inherently support distinct rules for 'new' vs 'old' imports, given that it evaluates files as they exist at a particular point in time without knowledge of their history. Can you confirm this? Or are there any workarounds to achieve a similar effect?

Yeah, it doesn't track git history.

My assumption was to scan for useMemo/useCallback usage in the file. If such usage is found (and it's used only one time), then we can show the error like "react-compiler optimizes everything for you". But if we have more occurrences, then we will not throw the error since we assume that the user understand what he is doing.

Maybe not ideal rule, but will give a clear instructions for new contributors.

how about we move over to slack with this discussion?

Yeah, let's do it! Feel free to start a new topic 😊 🙌 (I guess the old one was created a long time ago and it's hard to find it for most of the people).

kirillzyusko · 2024-07-04T09:11:35Z

@mountiny just a friendly ping to review these changes again 👀

mountiny

Sorry, been ooo, looks good to me but there is more conflicts now

kirillzyusko · 2024-07-05T09:04:23Z

but there is more conflicts now

@mountiny resolved all of them! Feel free to have a look again 😊

kirillzyusko · 2024-07-08T14:12:38Z

@mountiny just a friendly ping again 😅 Hopefully this PR will not get any conflicts in next several hours 😁

OSBotify · 2024-07-10T11:19:47Z

🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.6-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-07-10T11:37:01Z

🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.6-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-07-11T10:17:16Z

🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 9.0.6-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-07-11T10:36:52Z

🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 9.0.6-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-07-15T20:21:14Z

🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.6-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-07-17T00:18:55Z

🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.7-8 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

kirillzyusko marked this pull request as ready for review June 28, 2024 09:16

kirillzyusko requested a review from a team as a code owner June 28, 2024 09:16

melvin-bot bot requested review from Pujan92 and removed request for a team June 28, 2024 09:16

kirillzyusko force-pushed the feat/react-compiler-healthcheck branch from c5a201f to f1abccc Compare June 28, 2024 17:55

mountiny assigned kirillzyusko Jun 28, 2024

mountiny changed the title ~~feat: react-compiler healthcheck~~ [NoQA] feat: react-compiler healthcheck Jun 28, 2024

mountiny previously approved these changes Jun 28, 2024

View reviewed changes

mountiny removed the request for review from Pujan92 June 28, 2024 20:56

kirillzyusko dismissed mountiny’s stale review via 9945479 June 30, 2024 10:42

kirillzyusko force-pushed the feat/react-compiler-healthcheck branch from f1abccc to 9945479 Compare June 30, 2024 10:42

kirillzyusko force-pushed the feat/react-compiler-healthcheck branch from 3e633c4 to 81de55d Compare July 4, 2024 09:10

mountiny previously approved these changes Jul 5, 2024

View reviewed changes

kirillzyusko dismissed mountiny’s stale review via 10e966c July 5, 2024 08:44

kirillzyusko force-pushed the feat/react-compiler-healthcheck branch from a18f4d3 to 10e966c Compare July 5, 2024 08:44

kirillzyusko added 5 commits July 8, 2024 15:58

feat: react-compiler healthcheck

9b47620

fix: ignore react-compiler violations

467ec27

fix: last eslint violation

6ba51f9

chore: bump react-compiler version

19b1b5b

fix: changes after rebase

75abab0

kirillzyusko force-pushed the feat/react-compiler-healthcheck branch from 10e966c to 75abab0 Compare July 8, 2024 13:59

mountiny approved these changes Jul 8, 2024

View reviewed changes

mountiny merged commit fe2fcf0 into Expensify:main Jul 8, 2024
16 checks passed

This was referenced Jul 8, 2024

Investigate workflow job failing on main: lint / Run ESLint #45006

Closed

Investigate workflow job failing on main: confirmPassingBuild #45007

Closed

mountiny mentioned this pull request Jul 8, 2024

[NoQA] Skip react compiler check on NetSuiteApprovalAccountSelectPage #45012

Merged

50 tasks

github-actions bot mentioned this pull request Jul 10, 2024

Deploy Checklist: New Expensify 2024-07-10 #45144

Closed

ikevin127 mentioned this pull request Jul 15, 2024

[HOLD for payment 2024-07-22] [$500] Android & iOS-Chat - Report flickers, keyboard is dismissed when focus on composer on Android #45152

Closed

6 tasks

kirillzyusko mentioned this pull request Jul 24, 2024

[HOLD for payment 2024-08-19] Investigate improvements of the react-compiler #44384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NoQA] feat: react-compiler healthcheck #44460

[NoQA] feat: react-compiler healthcheck #44460

kirillzyusko commented Jun 26, 2024 •

edited

Loading

hannojg commented Jun 28, 2024

hannojg commented Jun 28, 2024 •

edited

Loading

melvin-bot bot commented Jun 28, 2024

kirillzyusko commented Jun 28, 2024 •

edited

Loading

mountiny commented Jun 28, 2024

kirillzyusko commented Jun 28, 2024

mountiny commented Jun 28, 2024 •

edited

Loading

mountiny commented Jun 28, 2024

kirillzyusko commented Jun 30, 2024

blazejkustra commented Jul 1, 2024

kirillzyusko commented Jul 1, 2024

blazejkustra commented Jul 2, 2024

kirillzyusko commented Jul 2, 2024

kirillzyusko commented Jul 4, 2024

mountiny left a comment

kirillzyusko commented Jul 5, 2024

kirillzyusko commented Jul 8, 2024

OSBotify commented Jul 10, 2024

OSBotify commented Jul 10, 2024

OSBotify commented Jul 11, 2024

OSBotify commented Jul 11, 2024

OSBotify commented Jul 15, 2024

OSBotify commented Jul 17, 2024

[NoQA] feat: react-compiler healthcheck #44460

[NoQA] feat: react-compiler healthcheck #44460

Conversation

kirillzyusko commented Jun 26, 2024 • edited Loading

Details

Most common mistakes

🔴 Ignored eslint rule

🔴 Modifying SharedValue (reactwg/react-compiler#14)

🔴 Modifying ref?

🔴 useNativeDriver is treated as a hook 😅

🔴 Props mutation (even if it's ref/SharedValue)

🔴 dependency list should be an array literal

🔴 mutating external variables

🔴 Writing to a variable defined outside

⚠️ Expected Identifier, got NumericLiteral key in ObjectExpression

🔴 complex manual memoization

🔴 Inferred dependencies did not match manually specified dependencies

⚠️ Expected Identifier, got MemberExpression key in ObjectExpression

🔴 Invalid nesting in program blocks or scopes

⚠️ Expression type ArrowFunctionExpression cannot be safely reordered

⚠️ Expected Identifier, got LogicalExpression key in ObjectExpression

⚠️ Handle Identifier inits in ForInStatement

⚠️ Expression type TSAsExpression cannot be safely reordered

⚠️ Handle UpdateExpression with MemberExpression argument

⚠️ Expression type CallExpression cannot be safely reordered

⚠️ Unexpected terminal kind optional for logical test block

⚠️ Expression type NewExpression cannot be safely reordered

⚠️ Handle computed properties in ObjectPattern

⚠️ Handle TSSatisfiesExpression expressions

⚠️ Handle tagged template with interpolations

⚠️ Unexpected terminal kind optional for ternary test block

⚠️ Expected Identifier, got TemplateLiteral key in ObjectExpression

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

hannojg commented Jun 28, 2024

hannojg commented Jun 28, 2024 • edited Loading

melvin-bot bot commented Jun 28, 2024

kirillzyusko commented Jun 28, 2024 • edited Loading

mountiny commented Jun 28, 2024

kirillzyusko commented Jun 28, 2024

mountiny commented Jun 28, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

mountiny commented Jun 28, 2024

kirillzyusko commented Jun 30, 2024

blazejkustra commented Jul 1, 2024

kirillzyusko commented Jul 1, 2024

blazejkustra commented Jul 2, 2024

kirillzyusko commented Jul 2, 2024

kirillzyusko commented Jul 4, 2024

mountiny left a comment

Choose a reason for hiding this comment

kirillzyusko commented Jul 5, 2024

kirillzyusko commented Jul 8, 2024

OSBotify commented Jul 10, 2024

OSBotify commented Jul 10, 2024

OSBotify commented Jul 11, 2024

OSBotify commented Jul 11, 2024

OSBotify commented Jul 15, 2024

OSBotify commented Jul 17, 2024

kirillzyusko commented Jun 26, 2024 •

edited

Loading

🔴 Modifying `SharedValue` (reactwg/react-compiler#14)

🔴 Modifying `ref`?

🔴 `useNativeDriver` is treated as a hook 😅

⚠️ Expression type `ArrowFunctionExpression` cannot be safely reordered

⚠️ Expression type `TSAsExpression` cannot be safely reordered

⚠️ Expression type `CallExpression` cannot be safely reordered

⚠️ Unexpected terminal kind `optional` for logical test block

⚠️ Expression type `NewExpression` cannot be safely reordered

⚠️ Unexpected terminal kind `optional` for ternary test block

hannojg commented Jun 28, 2024 •

edited

Loading

kirillzyusko commented Jun 28, 2024 •

edited

Loading

mountiny commented Jun 28, 2024 •

edited

Loading