Project refactoring

[MaksimLavrenyuk]

Multiple changes, according to https://github.com/jessetane/queue/issues/86:

Code Changes:

• the entire project, including tests, is rewritten to ESM modules
• tests rewritten on tap-esm
• the event mechanism was changed from using EventEmitter to EventTarget

Dependency changes:

• istanbul/nyc do not work with ESM, replaced by c8 which has esm support out of the box. It has support for nyc/istanbul reports - file lcov.info. Therefore, the travis script should not break.
• tsd-check is now just called tsd, updated it. The new versions updated expectType - now it's always a strict check, so I had to change it to expectAssignable https://github.com/SamVerschueren/tsd#strict-type-assertion
• To test in a browser, you need to pre-build the code, previously the build was done with Browserify. It has not been updated for 2 years and the project seems to be dead, so I replaced it with a rollup with minimal config.

There are several problems that require your attention:

• The change in the api typing is troubling. Previously, the typing files stated that Queue inherits from EventEmitter, now it inherits from EventTarget. If you maintain backward compatibility, you'll have to implement all the methods from EventEmitter, but now based on EventTarget - which would essentially be an implementation of your own EventEmitter.
• Since CustomEvent is only supported in nodejs since version 19, I had to create my own QueueEvent class instead of using the native CustomEvent. This creates detail typing problems. On the other hand, we previously used EventEmitter, which didn't have an indication for event typing. I've updated the readme with the new types, I think that's enough. Please indicate your opinion.

A question that needs confirmation Earlier in the discussion you suggested using only Promise. In order to avoid a difference of opinion on changes, I need to discuss this with you. As far as I understand it, this means not using callback. From that follows a change of methods, below I will describe what I have in mind, please pay attention to it, after your confirmation I will implement it.

Current typing:

class Queue {
    start (callback): void
}

interface QueueWorker {
    (callback?: QueueWorkerCallback): void;

    /**
     * Override queue timeout.
     */
    timeout?: number;
}

interface QueueWorkerCallback {
    (error?: Error, data?: Object): void;
}

Future typing

class Queue {
    start (): Promise<{ error?: Error, results?: any[] }>
}

type QueueWorkerResult = { error?: Error, data?: Object };

interface QueueWorker {
    ():Promise<QueueWorkerResult | void>;
    timeout?: number;
}

[jessetane]

Wow, awesome work! Unfortunately so much was changed in this PR (including directory structure) that it's challenging for me to review because the most of the important diffs are unavailable due to files being renames/moved. I wonder - would you be willing to break this up a bit for me? Maybe we can focus on the port to modern JS and ESM for the first PR (no comments in code please unless absolutely necessary, these just make the diffs I need to review more complicated). Removing dependencies in the first stage is fine with me, I am happy to see you remove standard, travis, coveralls, istanbul, browserify, typescript, really anything you can. If we need to add tooling back later that's fine, but we can do that in later PRs. Also, let's skip the build tooling for browser testing since it's not required - check out https://github.com/jessetane/hyperbind for an example. But yeah, in general if you can make the first PR as minimal as possible, without changing filenames or moving them it will make it much easier for me to review and get landed. If you get stuck on anything feel free to reach out and I'll try my best to help. Thank you again for your hard work!

[jessetane]

first PR should still include the switch from tape to tap-esm, perhaps obiviously..

[jessetane]

and, I see you asked a few typescript related questions but I don't use typescript so I can't help with these, sorry about that. either way this library needs to work without typescript so let's save that stuff for the end.

[MaksimLavrenyuk]

@jessetane

https://github.com/jessetane/queue/pull/90#issuecomment-1419775250 No problem, I'll let you know.

https://github.com/jessetane/queue/pull/90#issuecomment-1419787350 I apologize that I may have mislead you into thinking that the main problem is ts, the problem is changing the API of the library, and breaking backward compatibility. I pointed out the typing of the API change to work out some kind of agreement with you on the implementation of transferring the work to Promise, as well as changing the external API of the event layer. That way we can figure out which direction to go in without implementation. I can describe it in JSDoc notation if you feel more comfortable.

[jessetane]

Gotcha about the API change question, I personally don't care about backwards compat (that's what major versions are for) but maybe let's save the API changes for then next PR so we can discuss in isolation after all the modernizing/dep removal is done, what do you think?

[MaksimLavrenyuk]

@jessetane

I propose to divide PR into the following steps: 1) Refactoring to modern JS + ESM. Tests are transferred from tape to tap-esm. Dependence on events remains, detaching from it will be considered a backward compatibility violation. Changes that break backward compatibility should be done in a separate PR. Remove all dependencies, as well as travis and d.ts files. I have analyzed your approach at https://github.com/jessetane/hyperbind. I was concerned about the use of symbolic links, such as import hb from '/hyperbind/index.js'. This is not a standard path resolution in node.js modules, which is a major problem. I consider the lack of indication of this package by code editors to be an additional problem. Also, I tried running node test from the hyperbind folder and encountered the error Cannot find module '/hyperbind/index.js'. Since queue package doesn't use any browser api, I think it's critical to limit running tests to browser only. In the same PR I wanted to add c8 to collect coverage by tests, if the code is not run via node, this cannot be done. Yes, the coverage is not proof that the program works correctly, but in rewriting it helped me realize that I missed a few places, so it won't be superfluous. I don't think this is exactly what is required to run node_modules packages in a browser (this is the problem that needs to be solved). I think this needs to be solved in the following way: We need a static server serving the node_modules folder and the source code. When requesting files, we need to analyze the content before giving it to the browser, If the content is imported from node_modules, we need to replace the package name with the path to the package, taking into account the module/main/exports fields in package.json. For example:

• source file: import tap from 'tap-esm'
• The file the browser gets import tap from './node_modules/tap-esm/index.js' This task is non-trivial but interesting, I think I will try to implement it in the future. Unfortunately it will be beyond the scope of this PR series.

In view of the above, I suggest as a kind of intermediate step to still use build tools before launching in the browser.

2) Removing events. From this follows a violation of backward compatibility. Since EventTarget is used, I suggest the following (note payload):

/**
 * Since CustomEvent is only supported in nodejs since version 19,
 * you have to create your own class instead of using CustomEvent
 * @see https://github.com/nodejs/node/issues/40678
 * */
class QueueEvent extends Event {
  constructor (name, detail) {
    super(name)
    this.detail = detail
  }
}

q.dispatchEvent(new QueueEvent('start', { job }))
q.dispatchEvent(new QueueEvent('success', { result: [...result], job }))
q.dispatchEvent(new QueueEvent('error', { err, job }))
q.dispatchEvent(new QueueEvent('timeout', { next, job }))
q.dispatchEvent(new QueueEvent('end', { err }))

Adding a listener:

q.addEventListener('timeout', (event) => { event.detail.next() }) Moving from callback to promise, see https://github.com/jessetane/queue/pull/90#issue-1519290316

3) Adding tools. Additional tools like travis to maintain coveralls seems unnecessary, as well as coveralls itself. In tsd-check honestly do not see the point, it does not check the real js code, but only files index.d.ts, they can be anything. Without real compilation of ts => js you can't check correctness of types, everything else is imitation. I suggest you write the index.d.ts file yourself and don't check it with any package. Since I already did rewrite this package to TS it won't be a problem.

[jessetane]

1. You're right that this package needs to be testable in node and the browser, so hyperbind isn't the perfect example. Sorry about that. tap-esm itself is a better example - the solution is to use import-maps in the browser. 1.a. I do agree the symlink is less than ideal... why don't we just have the static file server point to the root of the project directory instead? Ecstatic has nice automatic directory listing already, and this way we could replace the test-browser and example scripts with a single script, maybe dev?
2. I am confused about the backwards compatibility issue you mention. The way I see it we are free to do what we want until we get around to cutting a release, at which point we will bump major/minor/patch according to semver. 2.a. QueueEvent sounds good to me, and don't worry about breaking the API, just do whatever makes sense to you at this stage and we'll be sure to bump the major version before we release.
3. 👍

[MaksimLavrenyuk]

Yes, import-maps seems like a good idea, I will try that.

[MaksimLavrenyuk]

@jessetane

I finished the first stage in the PR series, you can check.

node-ecstatic is closed as obsolete, so I replaced it with http-server. For more information, refer to this issue.

[jessetane]

Awesome. The diffs are still pretty rugged though, can you rebase your history so there are fewer commits and it's easier to read? git rebase -i e15babc6f54f20ad622ec1756d0f9a59d6d03164 might do the trick?

[MaksimLavrenyuk]

@jessetane I did it, but I did not notice a big difference with the previous diff, did I do something wrong?

[jessetane]

Hm, well there are 100 commits now, but the idea was to have fewer... if you're not familiar with interactive rebasing or don't have time to get the history nice, maybe just squash it all into a single commit for now?

[MaksimLavrenyuk]

@jessetane

https://github.com/jessetane/queue/pull/91