-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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
chore(create-vite): use non-null assertion #10772
Conversation
Not sure if the CI test failure is related. |
any reason this PR is marked as draft?
it's not. I re-ran it and it passed |
This is intentional: #7881 (comment) |
@patak-dev I'm aware of that, but as I explained, this is less safe and more confusing than using a non-null assertion. Can we please revert this? Edit: This seems to be an issue with TypeScript ESLint's config. TypeScript ESLint has a rule that explains why a non-null assertion is better. However, it's unfortunately only enabled in the strict config. I'm in the process of discussing this with upstream maintainers. |
Thanks Nick. Reopening while you discuss with them. |
now I feel responsible because I created that meme :) const root = document.getElementById('root')
if (!root) throw new Error('No root element found')
ReactDOM.createRoot(root).render(
<React.StrictMode>
<App />
</React.StrictMode>
) |
There is already a runtime check in createRoot if passed null. I don't think this manual check is worth it. I don't get why we the default rules disallow I will update the lint rules of the repo to add type ware rules and fix this at the same time |
The error message is
You mean you're adding ESLint configs to the create-vite templates? 😮 I'd like that. Let me know if you'd like help. |
My work on this is in pause for now, but maybe I could just try to change this one rule for this use-case before the next stable so that the 4.1 template uses |
Would it help if I tried to patch this in TypeScript ESLint's recommended config? Then we could just use that until the Vite specific lint config is ready (though I think their defaults are really good, excluding the |
I think they don't want to include type information for default rules. I not sure I will have time to do this today, so you can open a PR that just turn on this rule for the Vite repo |
Description
It's safer to use non-null assertions over unnecessary type assertions in TypeScript, since type assertions are bivariant.
Here are some practical advantages of using non-null assertions to render React elements:
as
which many TypeScript beginners may not understand the dangers ofElement
that isn't anHTMLElement
, such as<svg>
/SVGSVGElement
Element
, such asElement | boolean
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).