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

typescript definitions #1

Open
g5becks opened this issue Sep 4, 2020 · 21 comments · May be fixed by #2
Open

typescript definitions #1

g5becks opened this issue Sep 4, 2020 · 21 comments · May be fixed by #2

Comments

@g5becks
Copy link

g5becks commented Sep 4, 2020

Hi, cool library.

Are there any plans to add typescript definitions?

Thanks.

@wdavidw
Copy link
Member

wdavidw commented Sep 4, 2020

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
Copy link

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
Copy link
Member

wdavidw commented Dec 21, 2020

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

@LucasDemea
Copy link

LucasDemea commented Feb 1, 2023

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

@wdavidw
Copy link
Member

wdavidw commented Feb 1, 2023

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

@LucasDemea
Copy link

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

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
Copy link

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

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
Copy link
Member

wdavidw commented Feb 2, 2023

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
Copy link
Member

wdavidw commented Feb 2, 2023

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
Copy link
Member

wdavidw commented Feb 2, 2023

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
Copy link
Member

wdavidw commented Feb 2, 2023

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
Copy link

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

@LucasDemea
Copy link

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

@wdavidw
Copy link
Member

wdavidw commented Feb 6, 2023

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

@LucasDemea
Copy link

Didn't know this one, thanks

@LucasDemea
Copy link

One these lines

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
Copy link
Member

wdavidw commented Feb 6, 2023

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
Copy link

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
Copy link

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

@wdavidw
Copy link
Member

wdavidw commented Feb 6, 2023

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
Copy link
Member

wdavidw commented Feb 6, 2023

Is the call_sync function still in use

Yes, I use it in some libraries

@LucasDemea LucasDemea linked a pull request Feb 12, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants