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

chore(create-vite): react-ts non-null-assertion #7881

Merged
merged 3 commits into from
May 18, 2022

Conversation

mrskiro
Copy link
Contributor

@mrskiro mrskiro commented Apr 24, 2022

Description

hi!

I fixed the non-null assertion operator part of the react-ts template

why?

  • It is a typescript template, so it should be more robust

Using non-null assertion cancels the benefits of the strict null checking mode.

https://palantir.github.io/tslint/rules/no-non-null-assertion/

  • vite is a popular library and I hope it is a good example.
  • It is clear that the root element exists, but it is unclear from the codebase alone.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@mrskiro mrskiro changed the title Chore/react ts non null chore(create-vite)/react ts non null Apr 24, 2022
@mrskiro mrskiro changed the title chore(create-vite)/react ts non null chore(create-vite)/non-null-assertion Apr 24, 2022
@mrskiro mrskiro changed the title chore(create-vite)/non-null-assertion chore(create-vite): non-null-assertion Apr 24, 2022
@patak-dev
Copy link
Member

Thanks for the PR @purp1eeeee! Interesting problem.
We don't have this rule enabled in Vite's config. And in this case, I don't think that adding the guard makes things more clear. We know there is a root element in the app. @Shinigami92 do you know if we encountered situations like this in templates before? Since we don't configure eslint in the template, we depend on what users end up deciding to use. If this rule is common enough, we may want to a comment to ignore this line. Leaning towards not doing anything though.

@mrskiro
Copy link
Contributor Author

mrskiro commented Apr 24, 2022

Hmmm...
It is true that the eslint setting is not in this template. However, if it is a TypeScript template, the codebase alone does not tell us if this root element will always be present. So I think we need to add a guard with or without eslint

@Shinigami92
Copy link
Member

@typescript-eslint/no-non-null-assertion is in the recommended rule-set!

Beside that, using the !-operator in TS is like: compiler shut up, I know better

As this is controlled by the user and they just can remove

I would prefer a solution like @purp1eeeee provided with this PR

@bluwy
Copy link
Member

bluwy commented Apr 25, 2022

I'm also not sure about this PR, I prefer the previous code as it's more in line with the other templates and looks straightforward. I probably wouldn't want the extra code in production either as it's almost always guaranteed that the element exists.

@mrskiro
Copy link
Contributor Author

mrskiro commented Apr 25, 2022

Sorry. I have added to the Description why I submitted this PR

@aladdin-add
Copy link

aladdin-add commented Apr 25, 2022

another way is to use eslint-disable comment.

good to have a -- desc like:

/* eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- Here's a description about why this configuration is necessary. */

@Shinigami92
Copy link
Member

another way is to use eslint-disable comment.

good to have a -- desc like:

/* eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- Here's a description about why this configuration is necessary. */

I would be also okay with that as we are using this "strategy" already e.g. here:

// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/ban-types

@sapphi-red
Copy link
Member

FYI CRA uses as HTMLElement (this will not be warned by no-non-null-assertion).
https://github.com/facebook/create-react-app/blob/f99167c014a728ec856bda14f87181d90b050813/packages/cra-template-typescript/template/src/index.tsx#L7-L14
But there was no discussion about this (Perhaps it is the author's preference): facebook/create-react-app#12220

@mrskiro
Copy link
Contributor Author

mrskiro commented Apr 25, 2022

Hmmm...

I guess one way around this is to use eslint comments.

However, I am not sure why you are trying to work around it with comments.
I would rather fix it as TypeScript pointed out than write a comment (this PR).

@patak-dev
Copy link
Member

@purp1eeeee I think we can use as HTMLElement as suggested by @sapphi-red. It is the less noisy of the options.

@patak-dev patak-dev changed the title chore(create-vite): non-null-assertion chore(create-vite): react-ts non-null-assertion May 18, 2022
@patak-dev patak-dev merged commit 771312b into vitejs:main May 18, 2022
@mrskiro
Copy link
Contributor Author

mrskiro commented May 19, 2022

@patak-dev @Shinigami92
Sorry. I was too busy to work on it.
thank you!!!

@mrskiro mrskiro deleted the chore/react-ts-non-null branch May 19, 2022 04:09
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.

6 participants