remy / nodemon

Monitor for any changes in your node.js application and automatically restart the server - perfect for development
http://nodemon.io/
MIT License
26.26k stars 1.72k forks source link

Problem with Newley released types #2206

Closed KevinGruber closed 4 months ago

KevinGruber commented 4 months ago

Expected behaviour

When importing nodemon there is a default export for functionality defined in the types.

Actual behaviour

With the addition of https://github.com/remy/nodemon/pull/2204 there where types added to the project, but no default export types for functionality -> that leads to a not usable nodemon in typescript

Steps to reproduce

import nodemon from 'nodemon';

-> nodemon is not a module


remy commented 4 months ago

Just checking, but does require work? And can you import using `import * as nodemon from 'nodemon'?

KevinGruber commented 4 months ago

const nodemon = require('nodemon'); - WORKS doesn't load types

import * as nodemon from 'nodemon'; - DOESN't work

I am pretty sure it is because there is no export of the functionality of nodemon.

Based on the types nothing is exported

remy commented 4 months ago

I need to have a play. I only added types as an attempt to help people who were requiring the module, but I don't personally use TypeScript (some context). If you have any advice, please do share, otherwise I'll tinker this week đź‘Ť

remy commented 4 months ago

Okay, to clarify, import nodemon from 'nodemon' does work, i.e. this code works:

// test.mjs
import nodemon from 'nodemon'
nodemon({}).on('start', () => {
  console.log('started');
});

But the types aren't being found by tools like VS Code.

This is my code:

package.json

{
  "name": "2206",
  "version": "1.0.0",
  "main": "index.js",
  "dependencies": {
    "nodemon": "^3.1.1"
  }
}

index.js (the sub process):

console.log('Hello, World!');

And the run script: test.mjs

import nodemon from 'nodemon';

nodemon({}).on('start', () => {
  console.log('started');
});

From the command line:

SCR-20240528-jwdv

@KevinGruber you're saying it's undefined, can you share your code?

KevinGruber commented 4 months ago

Ok sry for the misunderstanding.

The runtime code works yes, but the typescript doesn't build because of "wrong" types.

undefined in the sense of types in TS

Beforehand it worked because there where no types (and I configured ts to allow for no types).

If you want I can make a PR with adjusted types?

remy commented 4 months ago

Yeah, if you could do a PR I'd appreciate it. I don't want to convert the project to typescript, but I do want to expose the types to help devs (which is why I've just added index.d.ts)

KevinGruber commented 4 months ago

I have the types already working, I now ran into the problem that they are not fitting the api approach types.

Can you help me and send me a list of allowed attributes? Like script etc.

are those than just merged with the current NodemonSettings? Or is NodemonSettings just the json config representation?

remy commented 4 months ago

This:

NodemonSettings just the json config representation

Sod, there's more props, I'll take a look and see if I can get a complete list. Annoyingly the --dump command doesn't quite list it out, because the inbound config is rearranged to the internal config (like script is moved to execOptions.

remy commented 4 months ago

If you have a branch, send up a work in progress PR and I can add to it (I think…)

remy commented 4 months ago

Found the following - pretty sure that's it.

script?: string; 
ext?: string; // "js,mjs" etc (should really support an array of strings, but I don't think it does right now)
events?: { [key: string]: string }; 
env?: { [key: string]: string };
exec?: string; // node, python, etc
execArgs?: string[]; // args passed to node, etc
github-actions[bot] commented 4 months ago

:tada: This issue has been resolved in version 3.1.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

arpowers commented 4 months ago

@remy the types still aren't correct in latest version, type errors happen when using the documented API (the API that was 100% working prior to 3.1.1.

KevinGruber commented 4 months ago

@arpowers what do you mean with working before, it wasn't using any types before

Can you show me the error?

remy commented 4 months ago

@arpowers a screenshot would be useful here, or othewise some code for context. Ta.

arpowers commented 4 months ago

Sure here are the errors that show up after upgrade:

Let me know if you want a link to the code.

@fiction/core/plugin-env/restart.ts:19:13 - error TS2503: Cannot find namespace 'nodemon'.

19     config: nodemon.Settings
               ~~~~~~~

@fiction/core/plugin-env/restart.ts:65:8 - error TS2339: Property 'on' does not exist on type '(settings: NodemonSettings) => Nodemon'.

65       .on('log', () => {})
          ~~
Screenshot 2024-05-29 at 1 37 13 PM
arpowers commented 4 months ago

I should mention that I'm using "@types/nodemon": "^1.19.6", as well, let me try and uninstall that and see if anything changes.

....and no change, type errors are still there.

remy commented 4 months ago

Oooh. I know what it is. Though, not sure how you represent it in Typescript. It's because the nodemon function returns itself, and it's supposed to be a singleton.

This isn't a fix, but if you chain the. on to the call to nodemon, I suspect it will work.

I'll see if I can fix the types in the morning, or if anyone gets a PR before then, happy to merge.

Unrelated: I wonder who is maintaining @types/nodemon...that isn't mine... 🤔

arpowers commented 4 months ago

no hurry, just locked version for now.

remy commented 4 months ago

@KevinGruber or @arpowers I'd appreciate some eyes on my change: https://github.com/remy/nodemon/pull/2208/files - I've tested it and it appears to work (with autocomplete), but more importantly the code actually still runs.

Again, I'm not a regular TS dev, so wary it could be wrong.

KevinGruber commented 4 months ago

@remy yes that works for me I tested both use cases

I wasn't aware that you can use nodemon as a singleton like that. Sry for missing that :-(

I noticed another thing yesterday can you provide string and string[] to nodeArgs and execArgs?

I took your suggestion and it took only string[] but my old code provided string.

If it supports both and is expected I would change the type to string | string[]

github-actions[bot] commented 4 months ago

:tada: This issue has been resolved in version 3.1.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

arpowers commented 4 months ago

@remy all working, it caused some type errors, but only because my code was wrong. Working exactly as it should. Thanks!

kpman commented 3 months ago

Unrelated: I wonder who is maintaining @types/nodemon...that isn't mine... 🤔

@remy The @types/nodemon author's information:

Type definitions for nodemon 1.19
Project: http://nodemon.io
Definitions by: Emily Marigold Klassen <https://github.com/forivall>
Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped

https://github.com/DefinitelyTyped/DefinitelyTyped/pull/39088

Maybe you could contact her or just porting the type definitions into this repo. The type definitions in 3.1.3 are not worked when using as the module.

remy commented 3 months ago

@kpman can you clarify this (or provide a working example)?

The type definitions in 3.1.3 are not worked when using as the module.

GabenGar commented 3 months ago

@remy Just FYI you can use arethetypeswrong to check if the types are declared properly.

remy commented 3 months ago

@GabenGar that just tells me what you told me, that they're wrong. I'm not a typescript person, and when I do use it (for work) it's a day of deciphering ts error messages.

So if you can (I'd hugely appreciate it) explain in plain English what's wrong, I can try to fix it.

GabenGar commented 3 months ago

If you are not a typescript person then why do you provide typescript types instead of delegating them to DefinitelyTyped? The problem at hand is not typescript per se but rather nodejs ESM/CJS shenanigans, and the explanation of the problem is as plain english as it can get, considering the subject matter. Your types say the package is a ESM module, when in reality it's CJS and the typescript need to know this distinction to work properly with nodejs ESM code. The distinction didn't matter back when ESM was just a mere syntactic sugar meant for feeding into a bundler and it just works™, but now nodejs and even browsers support ESM, and both are pretty anal about the rules.

arpowers commented 3 months ago

I got it all working fine w "preserve" as module resolution mode. It does error due to CJS/ESM confusion if it's set to "NodeNext"...