neutralinojs / neutralino.js

JavaScript API for Neutralinojs
https://neutralino.js.org/docs/api/overview
MIT License
237 stars 47 forks source link

NL_APPVERSION missing in type definitions #86

Closed jouni-kantola closed 7 months ago

jouni-kantola commented 11 months ago

The global property NL_APPVERSION is missing when installing @neutralinojs/lib or updating type definitions via npx @neutralinojs/neu update.

Workaround required is:

// @ts-expect-error
await window.setTitle(`App version v${NL_APPVERSION}`);
Sadaf-A commented 7 months ago

Hey @shalithasuranga, I looked into this issue and I found something

    if(!options["version"].is_null()) {
        jsSnippet += "var NL_APPVERSION='" + options["version"].get<string>() + "';";
    }

this is the part of the code that gets the app version but the if statement will not run if the value for version is null. I think we can solve this by providing a default value if the value of version is null. do you agree with this approach should i work on it further?

shalithasuranga commented 7 months ago

Hello @Sadaf-A, thanks for checking, this is done like that because version is optional for Neutralinojs apps. Btw, seems like we just need to add global TypeScript definition for this one by modifying global.d.ts?

Edit: seems like it's okay to add null for NL_APPVERSION too :)

Sadaf-A commented 7 months ago

Hello @Sadaf-A, thanks for checking, this is done like that because version is optional for Neutralinojs apps. Btw, seems like we just need to add global TypeScript definition for this one by modifying global.d.ts?

Yes, as I mentioned on discord we can make the NL_APPVERSION optional like this

NL_APPVERSION?: string;

would that be the right approach?

shalithasuranga commented 7 months ago

Mm.. Let's try to keep the implementation as it is and update the Ts file :)

shalithasuranga commented 7 months ago

Hello @Sadaf-A , seems like what we declare in global.d.ts are not embedded in index.d.ts. Could you please check whether entering window.NL_APPVERSION works file on VS Code intellisense? Thank you :)

Sadaf-A commented 7 months ago

Hello @Sadaf-A , seems like what we declare in global.d.ts are not embedded in index.d.ts. Could you please check whether entering window.NL_APPVERSION works file on VS Code intellisense? Thank you :)

Hey @shalithasuranga,

    window.NL_APPID
    window.NL_CVERSION
    window.NL_MODE
    window.NL_OS
    window.NL_PORT
    window.NL_VERSION

these are the properties being detected in the intellisense. should I look into why other properties are not being detected?

shalithasuranga commented 7 months ago

Hello, this was fixed in 5.0.1. Tested with the following scenarios:

  1. If app developers use /// <reference path="./neutralino.d.ts" /> in a TypeScript file, they won't see intellisence issues for the following statements:

    NL_VERSION;
    NL_APPVERSION;
    // ... all globals
  2. If app developers use the Neutralinojs client from NPM, they won't see issues for the following statements:

import { os } from '@neutralinojs/lib';
window.NL_VERSION;
window.NL_APPVERSION;
// .. all globals

@Sadaf-A Could you please check wether it also works for you? Thanks :tada:

Sadaf-A commented 7 months ago

Hello, this was fixed in 5.0.1. Tested with the following scenarios:

1. If app developers use `/// <reference path="./neutralino.d.ts" />` in a TypeScript file, they won't see intellisence issues for the following statements:
NL_VERSION;
NL_APPVERSION;
// ... all globals
2. If app developers use the Neutralinojs client from NPM, they won't see issues for the following statements:
import { os } from '@neutralinojs/lib';
window.NL_VERSION;
window.NL_APPVERSION;
// .. all globals

@Sadaf-A Could you please check wether it also works for you? Thanks 🎉

Hey, I'm not getting any TypeScript errors and intellisense is not showing NL_APPVERSION

kedodrill commented 6 months ago

@shalithasuranga I know this is closed, wouldn't think this merits a new issue...but if I add more globals via config (for example, NL_ENVIRONMENT or something), how would I add typescript definitions for that so the typescript compiler doesn't complain about it?

Sadaf-A commented 6 months ago

@shalithasuranga I know this is closed, wouldn't think this merits a new issue...but if I add more globals via config (for example, NL_ENVIRONMENT or something), how would I add typescript definitions for that so the typescript compiler doesn't complain about it?

I thinj the same way as we have added NL_APPVERSION in this PR

kedodrill commented 6 months ago

@shalithasuranga I know this is closed, wouldn't think this merits a new issue...but if I add more globals via config (for example, NL_ENVIRONMENT or something), how would I add typescript definitions for that so the typescript compiler doesn't complain about it?

I thinj the same way as we have added NL_APPVERSION in this PR

I ended up following this commit: https://github.com/neutralinojs/neutralino.js/commit/1f3604e24db9dc385fe3096d899d8d94d0596a77, and I added it in a globals.ts file I had been using for the app.