dargmuesli / nuxt-cookie-control

A highly configurable cookie banner for Nuxt.
MIT License
229 stars 43 forks source link

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

Open Yizack opened 2 months ago

Yizack commented 2 months ago

πŸ“š 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

https://github.com/dargmuesli/nuxt-cookie-control/blob/a54e6cb0edde969b54dc4268d3a426d6f13f5d99/src/runtime/components/CookieControl.vue#L380

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

dargmuesli commented 2 months ago

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

Yizack commented 2 months ago

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 commented 2 months ago

This way works, what do you think?

const headElement = document.getElementsByTagName('head')[0]
if (!headElement) return;
headElement.appendChild(script)
dargmuesli commented 2 months ago

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 :joy: Do brackets help by any chance? Like if (!(document.getElementsByTagName('head')[0])) return.

Yizack commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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

Yizack commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

I think this is the corresponding issue:

Also potentially related:

dargmuesli commented 2 months ago

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 commented 1 week ago

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