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

Restrict usage of global React object #3376

Open
wants to merge 1 commit into
base: next
Choose a base branch
from
Open

Conversation

SebiVPS
Copy link
Contributor

@SebiVPS SebiVPS commented Feb 7, 2025

Description

follow up PR for the restriction of the React barrel import: also restrict the usage of the global React object.

@SebiVPS SebiVPS force-pushed the restrict-global-react branch 3 times, most recently from db79eae to c3672b2 Compare February 7, 2025 08:08
@SebiVPS SebiVPS marked this pull request as draft February 7, 2025 08:12
@SebiVPS SebiVPS force-pushed the restrict-global-react branch from c3672b2 to 3322719 Compare February 7, 2025 08:21
@SebiVPS SebiVPS force-pushed the restrict-global-react branch from 3322719 to 171cb4b Compare February 7, 2025 08:33
@SebiVPS SebiVPS marked this pull request as ready for review February 7, 2025 08:37
@@ -1,5 +1,6 @@
/// <reference types="@comet/admin-theme" />

// eslint-disable-next-line no-restricted-globals
Copy link
Contributor Author

@SebiVPS SebiVPS Feb 7, 2025

Choose a reason for hiding this comment

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

I could not get it to work, when removing the React. usage here.

When its removed, there is an error, that the ref prop is missing in CustomInputProps.
And when the two types are imported import { type DetailedHTMLProps, type InputHTMLAttributes } from "react"; there is an error, that the added prop webkitdirectory is not found.

Do you have an idea what's going on here? Can we leave the React. usage here in the vendor.d.ts file (in the typedefinitions React. usage seems common?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should work:

/// <reference types="@comet/admin-theme" />

import { type HTMLAttributes } from "react";

declare module "react" {
    interface InputHTMLAttributes<T> extends HTMLAttributes<T> {
        // extends React's HTMLAttributes
        directory?: string;
        webkitdirectory?: string;
    }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we leave the React. usage here in the vendor.d.ts file (in the typedefinitions React. usage seems common?)

We don't need it in other .d.ts files 🤔 Maybe we should add explicit imports to all .d.ts files, otherwise probably the namespace is still used?

Copy link
Collaborator

@johnnyomair johnnyomair left a comment

Choose a reason for hiding this comment

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

Looks good. We need to decide how to handle *.d.ts files though.

@johnnyomair johnnyomair changed the title Restrict usage of global react object Restrict usage of global React object Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants