typescript definitions

[g5becks]

Hi, cool library.

Are there any plans to add typescript definitions?

Thanks.

[wdavidw]

Like in most in my libraries, I welcome contributions in this regard and then try to maintain those but I am not a TypeScript user, sometimes I need help to express those types.

[saugatdai]

Someone, please provide typescript definitions for this package, in my opinion, this would be a very useful plugin for some startups who want their system be extensible.

[wdavidw]

I would appreciate some help in this matter. I am not a TypeScript user nor an expert.

[LucasDemea]

Hi,
I can help with writing the types. Would you rather have the library rewritten in typescript, or only added types definitions ?

[wdavidw]

I have been hesitating, as a matter of fact. If you wish to go full TS, why not.

[LucasDemea]

I'm working on the TS rewrite and this line bugs me.

node-plug-and-play/lib/index.js

Line 122 in 89a80c6

return plugin.hooks[name].map(function(hook){

Could you please explain ?
Isn't plugin.hooks[name] supposed to refer to a single hook ? How come we could map this ?

[LucasDemea]

Not so sure about this[key] in this line :

node-plug-and-play/lib/error.js

Line 25 in 89a80c6

this[key] = Buffer.isBuffer(value) ? value.toString() : value === null ? value : JSON.parse(JSON.stringify(value));

Is there any reason for not simply doing key = ... ?

I'd need some help to make the tests work as I'm not familiar at all with mocha and currently it won't run:

Error: Use CoffeeScript.register() or require the coffeescript/register module to require .coffee files.

[wdavidw]

return plugin.hooks[name].map(function(hook){

It is always an array. the property plugin.hooks[name] is normalized at line 81 with the normalize_hook function.

[wdavidw]

Not so sure about this[key] in this line :

This refers to the error object instance. In here, we add any property of the contexts arguments to the error instance. For example, const err = new Error('CODE', 'some message', { prop: 'value' }) leads to err.prop === 'value'. Note, a commited a change to the error JS file to clean up the variable (a leftover from the coffee to js migration).

[wdavidw]

I'd need some help to make the tests work as I'm not familiar at all with mocha

The error is related to mocha not calling the Node.js --loader argument to load .coffee file, i'll have a look, I have done it in the past with the csv parser.

Note, I fixed the sample test, you'll have to rebase your branch.

[wdavidw]

Error: Use CoffeeScript.register() or require the coffeescript/register module to require .coffee files.

Replace all occurrences of ../lib/index.js with ../dist/esm/index.js in the tests and they will run. I just tested after a rebase on master. However, I am not against rewriting the test in TS but I can do this, it will train me with TS, I have too little experience with it.

[LucasDemea]

Thank you, I'll have a look at all this, this afternoon.

[LucasDemea]

Do you mind if we replace the array_flatten method with lodash.flatten, as it is already typed ?

[wdavidw]

I am reluctant to introduce dependencies, but there is arr.flat(Infinity) now which shall do the job.

[LucasDemea]

Didn't know this one, thanks

[LucasDemea]

One these lines

node-plug-and-play/lib/index.js

Lines 141 to 143 in 1f4b16b

	for(const hook of hooks){
	index[hook.plugin] = hook;
	}

We use the plugin property that is added to hooks a few lines before.

2 questions:

• This property isn't available on all hooks. Hooks coming from L107 don't have this property, can you confirm ?
• Should we add the require and plugin properties to the Hook interface, or just leave it as a transitional property used only for sorting ?

The Hook interface looks like this atm:

export interface Hook {
  /**
   * List of plugin names with hooks of the same name are to be executed after, a string is coerced to an array.
   */
  after?: string[];
  /**
   * List of plugin names with hooks of the same name are to be executed before, a string is coerced to an array.
   */
  before?: string[];
  /**
   * Name to indentify the hook.
   */
  name: string;
  /**
   * The hook handler to be executed
   */
  handler: HookHandler;

  // require?: string[];
  // plugin?: string;
}

[wdavidw]

I am trying to refresh my mind but I am pretty sure you are right, require and plugin look like transitional properties.

hook.plugin is the associated plugin name, and hook.require is used to get the list of required plugins defined in its associated plugin

I just push a commit on master to correct line 142.

[LucasDemea]

Thanks.
This part is a pain to adapt to typescript, as it goes against TS strong typing practice. In the TS paradigm, we should not dynamically add properties to the Hook interface. I found a workaround, but it's not the cleanest way. This will be a point of improvement in the future

[LucasDemea]

Is the call_sync function still in use ? You don't speak about it in the docs nor in your blog post.

[wdavidw]

we should not dynamically add properties to the Hook interface

Then you can always declare those properties. If I remember correctly, the idea is that user-provided hooks don't need a name and required plugins but I am not even sure anymore.

[wdavidw]

Is the call_sync function still in use

Yes, I use it in some libraries

[LucasDemea]

Hi @wdavidw Is there any plan to publish the latest version to npm ?

[wdavidw]

Sure, before that, there are 2 topics for which I would love your opinion.

1. I have some warning in Zed in many ts project, something like Cannot find module 'ssh2-connect' or its corresponding type declarations. There are types at '<source_pkg>/node_modules/<deps_pkg>/dist/index.d.ts', but this result could not be resolved under your current 'moduleResolution' setting. Consider updating to 'node16', 'nodenext', or 'bundler'.

Do you have experience passing the tsconfig to "moduleResolution": "bundler", would you anticipate any problem. It certain solve my editor warning

2. After plug-and-play, I migrate all my ts project to tsup, it is a lot easier to setup, almost brainlest. Are you ok with that, any feeback ?. Note, I could do this today or early next week.

[LucasDemea]

I think you can go on with 'nodenext' or 'bundler', these are the defaults for most projects.

I never used tsup, but it seems widely used. I wonder if it allows fine tuning as rollup does. I usually prefer configuring thing manually and having control on the bundle process. But this is my taste, and I don't really have an opinion on tsup as I never used it.

[wdavidw]

It is certainly not as flexible as rollup. For example, I don't think it generates iife nor umd bundlers. It is however possible to do the basic, here is an sample to give you an idea of the options:

export default defineConfig({
  entry: ["src/index.ts", "src/promises.ts"],
  outDir: "dist",
  clean: true,
  format: ["esm", "cjs"],
  target: ["esnext", "esnext"],
  dts: true,
  minify: false,
  sourcemap: true,
  splitting: false,
});

[wdavidw]

I just published version 2.6.0 but did not yet test its integration as a dependencies (exports are modified by tsup). Let me know if you encounter any issue.

[LucasDemea]

Great, will try that asap. Thanks for the tsup example.