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

Quasar plugin config property is typed incorrectly #16046

Closed
yusufkandemir opened this issue Jul 5, 2023 Discussed in #16045 · 17 comments · Fixed by #16213
Closed

Quasar plugin config property is typed incorrectly #16046

yusufkandemir opened this issue Jul 5, 2023 Discussed in #16045 · 17 comments · Fixed by #16213
Assignees
Labels
area/typescript bug/2-confirmed We have reproduce the problem and confirmed that this is a bug. kind/bug 🐞 Qv2 🔝 Quasar v2 issues

Comments

@yusufkandemir
Copy link
Member

Discussed in #16045

Originally posted by ghiscoding July 5, 2023
I have global configs defined for Quasar and that used to work properly before updating (mostly because the config property of QuasarPluginOptions was set to any so we could provide anything we wanted really)

So considering this global config

const quasarOptions: Partial<QuasarPluginOptions> = {
  // Quasar plugins
  plugins: {
    Dialog,
    Notify,
  },
  config: {
    /* Notify defaults */
    notify: {
      position: 'top',
      timeout: 2500,
    },
  },
};

const app = createApp(App);
app.use(Quasar, quasarOptions)

this no longer work and tells me what notify is not a known property of QuasarPluginOptions and seem to be caused by this PR #15945

Type '{ notify: { position: string; timeout: number; textColor: string; actions: { icon: string; color: string; }[]; }; loading: { spinner: ComponentConstructor; ... 4 more ...; customClass: string; }; }' is not assignable to type 'QuasarUIConfiguration'.
Object literal may only specify known properties, and 'notify' does not exist in type 'QuasarUIConfiguration'.ts(2322)
plugin.d.ts(13, 3): The expected type comes from property 'config' which is declared here on type 'Partial'

image

I am not expecting any errors since even the documentation still mentions the same way to configure the global configs

https://quasar.dev/quasar-plugins/notify#installation

image

Am I doing something wrong or is that a Regression Bug?

@yusufkandemir yusufkandemir self-assigned this Jul 5, 2023
@yusufkandemir yusufkandemir added kind/bug 🐞 area/typescript Qv2 🔝 Quasar v2 issues bug/2-confirmed We have reproduce the problem and confirmed that this is a bug. labels Jul 5, 2023
@robinpilz
Copy link

@yusufkandemir
If I'm correct this fix should be in v2.12.3 right? I just updated to that version but the issue still seems to persist or am I missing something here?

No overload matches this call.
Overload 1 of 2, '(plugin: Plugin<[options: Partial]>, options: Partial): App', gave the following error.
Overload 2 of 2, '(plugin: Plugin<[options: Partial]>, options: [options: Partial]): App', gave the following error.ts(2769)
plugin.d.ts(13, 3): The expected type comes from property 'config' which is declared here on type 'Partial'

And the type itself also seems to be missing the generated types:

export interface QuasarUIConfiguration {
  // These two are oddly structured and doesn't fit the API structure, so they don't have API definitions
  capacitor?: NativeMobileWrapperConfiguration;
  cordova?: NativeMobileWrapperConfiguration;

  // The rest will be augmented by auto-generated code
}

@yusufkandemir
Copy link
Member Author

Yes, the fix is released. But, share your code so we can check it.

And the type itself also seems to be missing the generated types

To understand that you would need to search for QuasarUIConfiguration in index.d.ts where it's augmented.

@robinpilz
Copy link

robinpilz commented Jul 21, 2023

@yusufkandemir
So in the index.d.ts it seems to be generated correctly, here is an excerpt:
2023-07-21 12_29_27-index d ts

But it does not seem to be augmented correctly when registering quasar. Here's the code I used for initializing:

import { boot } from '$utils/boot';
import { Dark, Dialog, Loading, LoadingBar, Notify, Quasar } from 'quasar';

export default boot(async ({ app }) => {
  app.use(Quasar, {
    plugins: {
      Loading,
      Dark,
      Notify,
      Dialog,
      LoadingBar,
    },
    config: {
      loadingBar: {
        color: 'primary',
        size: '2px',
      },
      notify: {
        position: 'top',
        actions: [
          {
            icon: 'close',
            color: 'white',
            round: true,
            dense: true,
            class: 'q-ml-md',
          },
        ],
      },
      dark: false,
    },
  });
});

I upgraded from v2.12.0 and there it was working, maybe this helps

@yusufkandemir
Copy link
Member Author

actions isn't listed under Quasar conf options in the docs https://quasar.dev/quasar-plugins/notify#notify-api. So, that's why actions causes a problem in your types. I tested it and it works on runtime, so it should be documented which will make it have generated types.

@robinpilz
Copy link

@yusufkandemir
This might also be the case but I of course also tried it with a minimal config with only the loadingBar and it still shows the error mentioned even if color and size are typed according to QuasarUIConfiguration. Seems like it worked previously as the config wasn't typed at all.

Interesesting though that it seems to work on your end. Is there something I could have missed in the setup, but the types should be picked up automatically I would assume

@danielwwc
Copy link

@yusufkandemir
The issue still persists on 2.13.3. Both config.loading and config.notify is not working

Object literal may only specify known properties, and 'loading' does not exist in type 'QuasarUIConfiguration'.

Screenshot 2023-07-24 at 11 54 21

@yusufkandemir
Copy link
Member Author

It works fine, make sure you have updated Quasar and the version gets picked up correctly. You can confirm with pnpm why quasar or through equivalent commands/methods of your package manager.

image

image
image

$ pnpm create vite
...

✔ Project name: … quasar-16046-test
✔ Select a framework: › Vue
✔ Select a variant: › TypeScript
...

$ cd quasar-16046-test

$ pnpm install

$ pnpm add quasar

Some options like spinner do not accept Component. If you are not using Quasar CLI, you can use @ts-expect-error/@ts-ignore to force typing as it will work on runtime.

@ghiscoding
Copy link

The issue still persists on 2.13.3. Both config.loading and config.notify is not working

Probably a typo here but there doesn't seem to be any version with that number, it should be 2.12.3

Installing 2.12.3 did work for me as well but it's missing a property in notify config, the actions should be valid but it's not and I don't see it either in the docs but it really does work so it should be added to the docs I think
https://quasar.dev/quasar-plugins/notify/#installation

image

with the config and ts-ignore, I see my close button (the "X" on the far right)

image

but if I remove the config, then my close button isn't showing anymore

image

@yusufkandemir
Copy link
Member Author

@ghiscoding #16102

@ghiscoding
Copy link

@yusufkandemir awesome thanks a lot for the reference, just a side note though, that PR fixes the Types but not the docs right?
https://quasar.dev/quasar-plugins/notify/#installation

image

@yusufkandemir
Copy link
Member Author

@ghiscoding it fixes both

@ghiscoding
Copy link

great, thanks for confirming :)

@danielwwc
Copy link

@yusufkandemir
The config.loading.spinner should be a component instead of string.
It is not working for string, I tried all possible values such as 'QSpinner', 'QSpinnerAudio', 'QSpinnerCube'.
Even if it works, could it be literal type?

It is working when passing a component but type-safe is complaining.

config: {
    loading: {
      spinner: 'QSpinner', // Not working, it is rendering nothing.
      backgroundColor: 'transparent',
      spinnerColor: 'primary',
    }
}

@rstoenescu
Copy link
Member

@danielwwc This IS working. Can you share a reproduction repo/codesandbox where it is not working pls?

@danielwwc
Copy link

@rstoenescu It is not working.
The config.loading.spinner should be a component instead of string. The example in the documentation is incorrect.
Here is the codepen: https://codepen.io/danielwwc/pen/mdQoqWR

config: {
    loading: {
      spinner: 'QSpinner', // Not working, it is rendering nothing.
      spinner: MyCustomSpinner, // this is working but typescript is complaining.
      backgroundColor: 'transparent',
      spinnerColor: 'primary',
    }
}

@rstoenescu rstoenescu reopened this Aug 5, 2023
@rstoenescu
Copy link
Member

@danielwwc Due to a clear reproduction I now see what the issue is. I was referring to quasar.config.js setting where String is required, as opposed to the UI code where a Component should be used instead. My next comment will explain why.

@rstoenescu
Copy link
Member

@yusufkandemir

There are subtle differences between quasar.config.js usage and the UI code (eg. a boot file).
For quasar.config.js we are running under a Node context, so we can't directly supply Components or action handlers (onClick etc). This is due to how the config is serialized and then used in the UI. As an example, config > notify > spinner must be a String in quasar.config.js which Quasar CLI detects and writes UI code for it (converts the String to a Component import in the UI space).
Now, for the UI space, user is supposed to specify a Component directly (he/she imports it manually and supplies it as config > notify > spinner), which is exactly what the CLI does under the hood.

There are also other Quasar plugins which have different config, based on where you are specifying it (quasar.config.js or UI space). Example: the "actions" that you pushed. You can't specify the "handler" in quasar.config.js because that's a Function and cannot be serialized when injected into the UI space.

As a general rule of thumb, it is really a bad idea to import anything UI related in the Node context of quasar.config.js (Components, Functions, etc). Can you focus on solving this, please? Make a diff between what the typings were before your main/first PR (because they were done correctly) and what they are now and implement a way to differentiate the things I mentioned above. Unfortunately, all Quasar plugins should be reviewed (which is where the diff will help).

rstoenescu pushed a commit that referenced this issue Aug 18, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…asarConfOptions (fix: #16046) (#16213)

* fix: define both UI and quasar.config file compatible types in quasarConfOptions

* feat(docs): display both UI and quasar.config file compatible types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/typescript bug/2-confirmed We have reproduce the problem and confirmed that this is a bug. kind/bug 🐞 Qv2 🔝 Quasar v2 issues
Projects
None yet
5 participants