fix: circular dependencies

[tpraxl]

This commit addresses circular dependencies between index.js and promise.js, as well as within files in the lib directory such as pool.js and pool_connection.js.

It introduces a new common.js module to centralize shared functionalities, thus breaking the direct dependencies among these files.

This approach resolves issues with the @opentelemetry/instrumentation-mysql2 package, which was losing several exports due to the circular dependencies.

You can reproduce the problem with this simple test repo:

https://github.com/tpraxl/test-esm-mysql-otel

Follow the instructions in the README.md to see the respective warnings.

To verify that this PR fixes the issue, check it out, run npm i && npm pack and install the fixed version into the test repo using npm i $PATH_TO_MYSQL2/mysql2-3.11.3.tgz. Run npm start again in the checked out test repo to see the warnings vanish.

This PR would fix https://github.com/open-telemetry/opentelemetry-js-contrib/issues/1511

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 81.31313% with 333 lines in your changes missing coverage. Please review.

Project coverage is 88.23%. Comparing base (d94bfaa) to head (623e8c9). Report is 69 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/base/connection.js	82.64%	164 Missing :warning:
lib/promise/connection.js	63.96%	80 Missing :warning:
lib/base/pool.js	87.55%	29 Missing :warning:
promise.js	55.35%	25 Missing :warning:
lib/promise/pool.js	84.82%	17 Missing :warning:
lib/base/pool_connection.js	84.12%	10 Missing :warning:
lib/promise/prepared_statement_info.js	78.12%	7 Missing :warning:
lib/pool_connection.js	66.66%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3081      +/-   ##
==========================================
+ Coverage   88.13%   88.23%   +0.10%     
==========================================
  Files          71       83      +12     
  Lines       12890    12983      +93     
  Branches     1355     1370      +15     
==========================================
+ Hits        11361    11456      +95     
+ Misses       1529     1527       -2     

Flag	Coverage Δ
compression-0	88.23% <81.31%> (+0.10%)	:arrow_up:
compression-1	88.23% <81.31%> (+0.10%)	:arrow_up:
tls-0	87.62% <78.73%> (+0.07%)	:arrow_up:
tls-1	87.99% <81.31%> (+0.10%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[wellwelwel]

Thanks, @tpraxl 🚀

I remember experiencing an error with rollup because of this.

Just a small recommendation, moving from ./common.js to ./lib/common.js 🙋🏻‍♂️

[tpraxl]

@wellwelwel You have a point there. In that case however, it feels weird to arbitrarily stuff some exported functions together in lib/common.js .

It made sense when we were talking about commons for index.js and promise.js, but in lib, I would split common.js up into the following files and get rid of common.js

• lib/create_connection.js
• lib/create_pool.js
• lib/create_pool_cluster.js

I'm working on it and will push the result

[wellwelwel]

Hey @tpraxl, I tested your changes with rollup:

---

• index.js

import mysql from 'mysql2/promise';

mysql.createPool({});

• rollup.config.js

import { defineConfig } from 'rollup';
import { nodeResolve } from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';
import json from '@rollup/plugin-json';

export default defineConfig({
  input: 'index.js',
  output: {
    file: 'dist.js',
  },
  plugins: [nodeResolve(), commonjs(), json()],
});

• package.json

{
  "type": "module",
  "devDependencies": {
    "@rollup/plugin-commonjs": "^28.0.0",
    "@rollup/plugin-json": "^6.1.0",
    "@rollup/plugin-node-resolve": "^15.3.0",
    "rollup": "^4.24.0"
  },
  "dependencies": {
    "mysql2": "file:../node-mysql2"
  }
}

---

I'll try to investigate this further.

[tpraxl]

Thanks for your finding. No need to investigate further IMHO, I would propose to extract PromiseConnection, PromisePool and PromisePoolConnection into lib. Each in its own file and to resolve the circular dependency that way.

See https://github.com/tpraxl/node-mysql2/pull/1/files

If these changes are OK for you, I'll merge them into fix/circular-dependencies and update this PR.

[tpraxl]

@wellwelwel are you fine with my changes? See https://github.com/tpraxl/node-mysql2/pull/1/files I would merge them into this PR if you are ok with it.

[mknj]

I am facing the same opentelemetry issue and i verified that this MR fixes it.

Please merge this MR.

[wellwelwel]

@wellwelwel are you fine with my changes?

@tpraxl, sorry for the delay (I've been away for a few days).

Testing in the new branch, I'm facing this now:

Previously there were three warnings 💡

[wellwelwel]

Please merge this MR.

@mknj, these refactors involve some critical files and modules, so it's ideal to review them carefully 🙋🏻‍♂️

Please, feel free to contribute to the review 🤝

[tpraxl]

I'll investigate this further, just haven't had the time yet. Before, I only fixed the obvious circular dependencies until it was working for the instrumentation. Now, I'll use madge --circular ..

I fear, at a quick first glance, that this fix won't be possible without breaking changes. But give me some time to investigate.

[wellwelwel]

I'll investigate this further, just haven't had the time yet.

Thanks, @tpraxl 🤝

I'll try to play in your last changes too 🙋🏻‍♂️

[tpraxl]

FYI: This seem to be the critical relations:

[wellwelwel]

@tpraxl, I didn't expect it to be so complex. As the warning from Rollup doesn't crashes or block the compilation, I'm fine with the current stage of this PR.

Optionally, we can use a final commit as fix: improve circular dependencies 💭

Then, when #2803 moves on or a major release milestone comes out, we can discuss it with more flexibility. What do you think? If you agree, LGTM 🙋🏻‍♂️

[tpraxl]

I am struggling, because madge shows that in this branch, I actually increased the amount of circular dependencies when I untangled index.js / promise.js (because there are more files now).

The only version having less circular dependencies is https://github.com/tpraxl/node-mysql2/tree/fix/circular-dependencies-promise I managed to fix that version (it caused one test to fail, because I missed to patch methods when I extracted classes). This version only has one circular dependency, as far as madge reports.

However, more problems are popping up when you go down the rabbit hole.

See https://github.com/tpraxl/test-esm-mysql-otel/blob/main/README.md for a thorough description and a way to reproduce the problems.

Basically, with the new version, @opentelemetry will not activate instrumentation for mysql2 anymore, because it does not encounter the main module when used with esm. This would be only possible with a workaround, that I also provided in https://github.com/tpraxl/node-mysql2/tree/fix/open-telemetry-not-activated But I believe this is actually an @opentelemetry bug and should not be fixed in mysql2.

So, if we merge this PR now, instrumentation for esm will not crash anymore, it will simply be gone for esm users. Sigh.

I hope, I'll find some time to investigate the new issues and maybe I can find a way to trigger a fix.

[tpraxl]

My proposal after having thought about it again:

Let's merge https://github.com/tpraxl/node-mysql2/tree/fix/circular-dependencies-promise into this PR. Have it reviewed thoroughly before the merge. (Next week, I am planning to test this version with a real-life project.)

Live with the fact that instrumentation is still not working for the callback variant in esm scenarios. At least, it does not crash anymore when instrumented.

Next steps:

• I'll dive deeper into the @opentelemetry instrumentation problem (indirection needed for main module in esm) and try to propose a fix
• After the conversion to typescript you mentioned, we'll tackle the last circular dependency left

What do you think?

[wellwelwel]

Let's merge https://github.com/tpraxl/node-mysql2/tree/fix/circular-dependencies-promise into this PR.

I didn't get to review https://github.com/tpraxl/node-mysql2/pull/1 (I just did a quick compilation test).

What do you think?

@tpraxl, feel free to proceed 🙋🏻‍♂️

[tpraxl]

Merged fix/circular-dependencies-promise into this branch.

Summary: This PR fixes the crash for esm apps using the callback variant of mysql2 with open telemetry mysql2 instrumentation. Open telemetry mysql2 instrumentation is however not yet activated for the callback variant, because it does not detect the main module 'mysql2' yet. This is most likely a bug in open telemetry's require patch and needs to be investigated and fixed there. My own tests of the callback variant with a real-life application revealed no problems with this version of mysql2. A thorough review is advised anyway.

[wellwelwel]

@tpraxl, I'll ask for a few days to review it carefully. Thanks in advance 🤝

[tpraxl]

@mknj thanks for your findings. You are even better than npm run lint . That should have been my job. Sorry.

[tpraxl]

Good news: Actually reading the complete documentation of opentelemetry helps. :eyes: I found out that, in order to have proper support for instrumentation in esm, you need to use the node argument --experimental-loader=@opentelemetry/instrumentation/hook.mjs. This seems to be overlooked oftentimes.

I adjusted the test repo and added the fixed start commands: https://github.com/tpraxl/test-esm-mysql-otel

Also adjusted the README to document this issue.

So, with the fixed startup, we actually gain instrumentation for the callback mode as well and with this PR merged, the format function will be available as well.

[wellwelwel]

@tpraxl, can I make some minor adjustments directly to your branch?

---

☔️ Just a note on coverage: the lines that aren't covered aren't new, they've just been moved to other files, triggering the missing coverage.

[tpraxl]

@tpraxl, can I make some minor adjustments directly to your branch?

Of course, feel free.

[wellwelwel]

Good news:

Hey @tpraxl, I think I have a good news too:

No more circular dependencies at all ✨

• I'll run a few tests before committing, but in any case, all the current test suites are passing successfully.

[wellwelwel]

These are the remaining circular imports:

• https://github.com/sidorares/node-mysql2/blob/b27898cf3193db4da2c6bf0c97513ccc15a1c2b9/lib/connection.js#L159
• https://github.com/sidorares/node-mysql2/blob/b27898cf3193db4da2c6bf0c97513ccc15a1c2b9/lib/pool.js#L37
• https://github.com/sidorares/node-mysql2/blob/b27898cf3193db4da2c6bf0c97513ccc15a1c2b9/lib/pool_connection.js#L33

To fix it in a simple way, I just moved the original classes for each conflict to a new file, removing the promise method from all of them. All the moved classes are now "base" classes that can be extended by synchronous and asynchronous implementations.

For example:

connection.js:

'use strict';

const BaseConnection = require('./base_connection.js');

class Connection extends BaseConnection {
  promise(promiseImpl) {
    const PromiseConnection = require('./promise_connection.js');
    return new PromiseConnection(this, promiseImpl);
  }
}

module.exports = Connection;

• Promise implementation:

    // ...
    if (
      typeof BaseConnection.prototype[func] === 'function' && // ⬅️
      PromiseConnection.prototype[func] === undefined
    ) {
      PromiseConnection.prototype[func] = (function factory(funcName) {
        return function () {
          return BaseConnection.prototype[funcName].apply( // ⬅️
            this.connection,
            arguments,
          );
        };
      })(func);
    }
  }
  // ...

• The same idea for pool.js and pool_connection.js.

[wellwelwel]

After almost 3 months, finally, I believe it's ready to merge 🚀

---

Not for this PR (which is already long enough), but I believe it's worth adding an E2E test (similar to the one I carried out with Rollup) to ensure that after all our efforts, this conflict doesn't occur again.

Thanks again, @tpraxl and @mknj 🤝

[wellwelwel]

It's already available in the 3.11.5-canary.d5a76e6c version 🎉

As said in #3207, although no logic has actually been changed, as it's a relatively large refactoring of critical files, I intend to keep the canary version for at least a week before merging it to an official release.