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

fix(types): handle non-null assertion #222

Merged
merged 3 commits into from
Sep 13, 2024

Conversation

Yizack
Copy link
Contributor

@Yizack Yizack commented Jun 30, 2024

📚 Description

Hello!, I started type-checking one of my projects which makes use of this module and I noticed that my vue-tsc --noEmit command complains about two type-check errors coming from this module, the 1st one is about plugin types not being injected (I opened #221 solving the issue) and the 2nd one indicates that line below is possibly undefined

document.getElementsByTagName('head')[0].appendChild(script)

This PR suggests that if we know that the head array will never be undefined, then add a non-null assertion (!) to indicate it to the ts-plugin

Reproduction

I created a minimal reproduction here: https://stackblitz.com/edit/github-rdlsvu-jmfrqy?file=package.json

  1. Click on the link to open the reproduction and wait for the container to run the commands
  2. Look for the error in the terminal

image


Adding the non-null assertion seems to work, I tested it locally on my vscode, no errors showing up
image

📝 Checklist

  • All commits follow the Conventional Commit format
  • The PR's title follows the Conventional Commit format

@dargmuesli
Copy link
Owner

I'd prefer something like if (!document.getElementsByTagName('head')[0]) return. What do you think?

@Yizack
Copy link
Contributor Author

Yizack commented Jul 1, 2024

I'd prefer something like if (!document.getElementsByTagName('head')[0]) return. What do you think?

I'd agree but seems like it doesn't do it, very strange... maybe it's related to a problem with the vuejs language-tools ts-plugin or vue-tsc update

image

While ! or ? this does it

image

@Yizack
Copy link
Contributor Author

Yizack commented Jul 1, 2024

This way works, what do you think?

const headElement = document.getElementsByTagName('head')[0]
if (!headElement) return;
headElement.appendChild(script)

@dargmuesli
Copy link
Owner

dargmuesli commented Jul 1, 2024

maybe it's related to a problem with the vuejs language-tools ts-plugin or vue-tsc update

I tend to believe that's a possibility. My developer tools show that document.getElementsByTagName('head')[0] is of type HTMLHeadElement, with no undefined alternative.

Do you only get the linter issue in projects depending on this module? As I don't get any errors linting this module itself.

This way works, what do you think?

Wow, why does it 😂 Do brackets help by any chance? Like if (!(document.getElementsByTagName('head')[0])) return.

@Yizack
Copy link
Contributor Author

Yizack commented Jul 1, 2024

@dargmuesli

Yes! The issue happens only in projects depending on this module, not the module itself 😂

Wow, why does it 😂 Do brackets help by any chance? Like if (!(document.getElementsByTagName('head')[0])) return.

It seems like it does not, it's a strange ts check behavior

@dargmuesli
Copy link
Owner

Let's wait for a few days and see if some update to vue-tsc solves the issue. I had several issues with the last few versions. I'd expect their tests would've caught those, but yeah, here we are. If nothing happens, I'll debug further.

@dargmuesli
Copy link
Owner

Just wondering, do you still see this issue occur in v8.4.4 released yesterday?

@Yizack
Copy link
Contributor Author

Yizack commented Jul 2, 2024

Just wondering, do you still see this issue occur in v8.4.4 released yesterday?

Yes! I still do after v8.4.4, updated the reproduction

@dargmuesli
Copy link
Owner

Ok, so I see two issues:

  1. the one reported here seems to be related to future: { compatibilityVersion: 4 }, without it the linting goes fine for me
  2. then there is a second issue error TS7006: Parameter 'id' implicitly has an 'any' type. which started to appear with vue-tsc v2.0.24 (everything before that and starting with v2.0.20 has a bug), so downgrading to vue-tsc 2.0.19 for now also resolves that issue

Maybe the first issue should be reported in the nuxt repo so it is resolved before v4?

@dargmuesli
Copy link
Owner

dargmuesli commented Jul 4, 2024

Please also try to override the typescript version to be 5.4.5, e.g. for pnpm in the package.json like

  "pnpm": {
    "overrides": {
      "typescript": "5.4.5"
    }
  }

Then delete the lockfile and reinstall all dependencies (seems like override wouldn't downgrade all typescript versions in the lockfile without deleting it first). That might also be a workaround for now.

@Reeska
Copy link

Reeska commented Sep 5, 2024

Hi,
I have the same issue even with TypeScript 5.4.5, is it possible to merge this PR ?

@dargmuesli
Copy link
Owner

@Reeska have you set the future.compatibilityVersion in your Nuxt project to 4?

@dargmuesli dargmuesli merged commit 2b59576 into dargmuesli:master Sep 13, 2024
12 checks passed
dargmuesli-bot pushed a commit that referenced this pull request Sep 13, 2024
## [8.4.11](8.4.10...8.4.11) (2024-09-13)

### Bug Fixes

* **types:** handle non-null assertion ([#222](#222)) ([2b59576](2b59576))
@dargmuesli-bot
Copy link
Collaborator

🎉 This PR is included in version 8.4.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Reeska
Copy link

Reeska commented Sep 18, 2024

@Reeska have you set the future.compatibilityVersion in your Nuxt project to 4?

No, I didn't

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants