Resolve ESM/CJS Module import conflict by using the minified lru-cache #3038

robert-pitt-foodhub · 2024-09-10T23:37:11Z

Hey,

This PR updates the imports of lru-cache to to the minified exports, this seems to resolve the LRU import error I get under certain context, I have run the unit and integration tests, no issues observed either.

Creating PR to run E2E Tests suite

…e export.

codecov · 2024-09-10T23:41:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.13%. Comparing base (dee0c08) to head (63abf71).
Report is 13 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3038   +/-   ##
=======================================
  Coverage   88.13%   88.13%           
=======================================
  Files          71       71           
  Lines       12889    12890    +1     
  Branches     1352     1354    +2     
=======================================
+ Hits        11360    11361    +1     
  Misses       1529     1529

Flag	Coverage Δ
compression-0	`88.13% <100.00%> (+<0.01%)`	⬆️
compression-1	`88.13% <100.00%> (+<0.01%)`	⬆️
tls-0	`87.55% <100.00%> (+<0.01%)`	⬆️
tls-1	`87.89% <100.00%> (+<0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wellwelwel · 2024-09-11T00:26:42Z

Thanks, @robert-pitt-foodhub 🙋🏻‍♂️

Bringing your #2988 (comment) here.

I'm not sure I understand the difference. The dist/cjs/index.min.js (lru-cache/min) should be just a minified build of dist/cjs/index.js (lru-cache).

wellwelwel · 2024-09-11T00:45:25Z

If we follow the #2988 traces, the main conflict is that users install packages using different versions of lru-cache.

An example (not real, just to ilustrate):

npm ls lru-cache

├── lru-cache@11.0.1
├─┬ mysql2@3.11.1
│ ├── lru-cache@8.0.5
│ └─┬ named-placeholders@1.1.3
│   └── lru-cache@7.18.3

nuxt: lru-cache v11
mysql2: lru-cache v8
named-placeholders: lru-cache v7

✅ node command will assign the exports and imports.
❌ nuxt compiler will use lru-cache v11 from itself.

The same idea for Next, for example. Even if they don't import lru-cache directly, a dependency of theirs does. This also applies if the user installs packages that depend on different versions of lru-cache, such as importing mysql2.

robert-pitt-foodhub · 2024-09-11T00:53:35Z

I think the point your making between node and nuxt is that nuxt is might be doing a compilation step, similar to the build step within my screenshots on the other PR, it was not failing while executing within node directly but during esbuild / typescript transpiration the error was appearing.

This might be useful: isaacs/node-lru-cache#323

wellwelwel · 2024-09-11T00:55:11Z

"Why doesn't everyone upgrade to the latest version of `lru-cache`?"

The behavior in my previous comment is due to the high engine requirements.

I tried to use the latest lru-cache in Node.js v18.x.x and it works 100%, but they support only from Node.js 20.x.

The maintainers decide to release new versions of lru-cache as soon as a version of Node.js is no longer LTS, even if the project is indirectly compatible (possibly from lru-cache v10 onwards).

wellwelwel · 2024-09-11T01:01:45Z

When we move from theory to practice, the constant high engine requirements have a "price".

Not all npm package maintainers want to drop support for Node.js versions in order to a dependency, except in the case of vulnerabilities, as happened with nodemon.

This causes a cascading effect where each package installs a compatible version as far as the lru-cache keeps support in common, what brings us to these issues and PRs.

Sorry for making it sound like a trend 😅

I've explained each stage in a commentary to make it easier.

wellwelwel · 2024-09-11T01:15:10Z

@robert-pitt-foodhub, I'll keep this PR opened as an alternative for #2988, in case @sidorares doesn't approve it.

And again, thanks for your contributions. Feel free to discuss here or there 🤝

robert-pitt-foodhub · 2024-09-11T01:25:26Z

My thought process with this is that these next, nuxt, nixt and whatever comes out next week are all using a bundling layer, and I think the way the bundling layer is resolving the modules differers, I think bundlers are more interested in high level files, such as typescript where they are richer in metadata, and use modern bundling import/export resolution strategies.

For example, here's the logs (using NODE_DEBUG=module yarn build), which shows where it's looking for the module and I believe in what order, and this ultimatly loads the root index.js

MODULE 80661: load "/Users/robertpitt/Projects/foodhub/falcon/node_modules/lru-cache/package.json" for module "/Users/robertpitt/Projects/foodhub/falcon/node_modules/lru-cache/package.json"
MODULE 80661: looking for "/Users/robertpitt/Projects/foodhub/falcon/node_modules/lru-cache/index" in [
    "/Users/robertpitt/Projects/foodhub/falcon/node_modules/mysql2/lib/node_modules",
    "/Users/robertpitt/Projects/foodhub/falcon/node_modules/mysql2/node_modules",
    "/Users/robertpitt/Projects/foodhub/falcon/node_modules",
    "/Users/robertpitt/Projects/foodhub/node_modules",
    "/Users/robertpitt/Projects/node_modules",
    "/Users/robertpitt/node_modules",
    "/Users/node_modules",
    "/node_modules",
    "/Users/robertpitt/.node_modules",
    "/Users/robertpitt/.node_libraries",
    "/Users/robertpitt/.nvm/versions/node/v18.17.1/lib/node"
]
MODULE 80661: load "/Users/robertpitt/Projects/foodhub/falcon/node_modules/lru-cache/index.js" for module "/Users/robertpitt/Projects/foodhub/falcon/node_modules/lru-cache/index.js"

And when call directly via node node ultimately loads dist/cjs/index.js

MODULE 85470: looking for "lru-cache" in [
    "/Users/robertpitt/Projects/foodhub/falcon/node_modules/mysql2/lib/parsers/node_modules",
    "/Users/robertpitt/Projects/foodhub/falcon/node_modules/mysql2/lib/node_modules",
    "/Users/robertpitt/Projects/foodhub/falcon/node_modules/mysql2/node_modules",
    "/Users/robertpitt/Projects/foodhub/falcon/node_modules",
    "/Users/robertpitt/Projects/foodhub/node_modules",
    "/Users/robertpitt/Projects/node_modules",
    "/Users/robertpitt/node_modules",
    "/Users/node_modules",
    "/node_modules",
    "/Users/robertpitt/.node_modules",
    "/Users/robertpitt/.node_libraries","/Users/robertpitt/.nvm/versions/node/v18.17.1/lib/node"
]
MODULE 85470: load "/Users/robertpitt/Projects/foodhub/falcon/node_modules/mysql2/node_modules/lru-cache/dist/cjs/index-cjs.js" for module "/Users/robertpitt/Projects/foodhub/falcon/node_modules/mysql2/node_modules/lru-cache/dist/cjs/index-cjs.js"
MODULE 85470: Module._load REQUEST ./index.js parent: /Users/robertpitt/Projects/foodhub/falcon/node_modules/mysql2/node_modules/lru-cache/dist/cjs/index-cjs.js
MODULE 85470: RELATIVE: requested: ./index.js from parent.id /Users/robertpitt/Projects/foodhub/falcon/node_modules/mysql2/node_modules/lru-cache/dist/cjs/index-cjs.js
MODULE 85470: looking for ["/Users/robertpitt/Projects/foodhub/falcon/node_modules/mysql2/node_modules/lru-cache/dist/cjs"]
MODULE 85470: load "/Users/robertpitt/Projects/foodhub/falcon/node_modules/mysql2/node_modules/lru-cache/dist/cjs/index.js" for module "/Users/robertpitt/Projects/foodhub/falcon/node_modules/mysql2/node_modules/lru-cache/dist/cjs/index.js

Do we have a reproduction of the next / nuxt issue, as I can test to see if the issue also resolves then as well, as my thought process is that my you import the min path your forcing the bundler to create a new context, i.e rescan for package.json, and this force of isolation I think triggers the bundlers to maybe fall back to the default import process, as the bundler would be treating the min folder as something completly seperate to the ESM module as a whole.

wellwelwel · 2024-09-11T01:30:25Z

Do we have a reproduction of the next / nuxt issue, as I can test to see if the issue also resolves then as well.

If we upgrade lru-cache, dropping Node.js support, this issue specifically will be fixed.

In some traces, the community shared a workaround.

wellwelwel · 2024-09-11T01:38:05Z

this seems to resolve the LRU import error I get under certain context

Also, it's important to note that these changes are trying to fix an issue from lru-cache that is already fixed in next versions of it, but without sacrificing the engine support. This issue isn't due to mysql2, nuxt, esbuild, etc. 🙋🏻‍♂️

This is the main reason I opted to replace the lru-cache in #2988. Even if we drop the support for Node.js v20, we will still need a new breaking change each time lru-cache drop an engine support again.

robert-pitt-foodhub · 2024-09-11T01:41:07Z

Yes, but dropping support for an older version of node might not be required right now if this change of how you import the module does have the desired effect, I am blocked on releasing 3.11.1 to production due to the fact that I get this LRU constructor issue, I don't have any way around the issue on my, if the /min import works as expected in next, nuxt, then you might not need to drop support for older versions of mysql if this has the same effect.

I got a strong feeling that the reason why it works is because when node has to load a subfolder, it basically treats that subfolder as a different scope to the main package module, aka index.js / entry point.

wellwelwel · 2024-09-11T01:49:28Z

it basically treats that subfolder as a different scope to the main package module

It doesn't seem to be a real solution. Imports aren't in subfolders in version 8.x.x:

Even without the lru-cache/min import, if we upgrade lru-cache to its latest version, everything would work normally in theory, where we back to #3038 (comment).

wellwelwel · 2024-09-11T01:57:00Z

I am blocked on releasing 3.11.1 to production due to the fact that I get this LRU constructor issue

Although I can merge #2988 and have complete confidence in the lru.min package, this is a change that replaces a direct dependency of the project.

When I merge on my own, usually no structure or architecture is changed because of what I have merged, which is not the case with #2988.

That said, as MySQL2 is mainly maintained by @sidorares, I would like at least a "LGTM" or an "approve" to merge it.

robert-pitt-foodhub · 2024-09-11T01:58:25Z

I don't think there is a literal folder, I think it's a symbol that is introduced from the package.json#exports field

I noticed this when investigating #56261 and suspected this would happen. I don’t think it’s a bug per se, but I understand why it’s undesirable in this case. But I don’t know what rule you could make that would catch this. I think the main reason you’re expecting the inferred module specifier to be "lru-cache" is because a different file resolved to the symbol in question that way, but it’s not generally true that two files can use the same module specifier (even when it’s a bare specifier) to refer to the same file. That’s true for two files of the same module format in the same directory, but if the files were in different directories, we’d have to see if they have different package.jsons in scope or different node_modules directories in scope, and at that point we’re pretty much just starting from scratch to see what module specifier we can use to resolve to the target file. And if we’re not explicitly relying on a cache entry that tells us we can use "lru-cache", there seems to be no good reason why we shouldn’t infer "lru-cache/min".

Source: microsoft/TypeScript#56290 (comment)

Ill take more of a look into this tomorrow, as I am not clear on how to work around this problem yet, im already at the latest version of mysql2, Im not sure there is any typescript settings that will resolve the import issue, so far the only way I found to work is for mysql2 to import from the min export.

wellwelwel · 2024-09-11T02:24:50Z

Commented by @robert-pitt-foodhub in #2988 (comment).

Hey @wellwelwel, me again 😆, as I have upgraded to the latest version of mysql2 to resolve my hanging connections issue, I now am facing this issue and was looking for a solution, I have looked through the attached tickets and there's nothing that really is working for my setup, however there is some things that I have noticed.

I am using an AWS CDK project, where during the building of my infrastructure, CDK will invoke build tools such as Esbuild to compile the typescript to JS, and they seem to be acting differently, for example:

Calling my build step invokes esbuild bundling, which I think leads to this error:

The error is simple, the LRU is not a constructor because the value of require("lru-cache").default is not a class, this is confirmed below when I executed the direct .js file using node binary (note I am using v18 for both build, compile and runtime), there is no Error throw, and this is because the LRU is imported correctly.

I took at look at what the outputs of require('lru-cache') were for each scenario:

Node:

Via bundling process

Ok, so I can see that there is a difference in what's logged between each environment, however, before I was going to jump down the rabbit hole of the typescript, bundlers loaders etc, I checked out lru-cache's package.json, and noticed they are exporting a min folder that mapps require to the correct common js module format

So I updated the import statements for both strings.js and parser_cache.js so that I import from lru-cache/min, such as const LRU = require('lru-cache/min').default;, and doing this fixes the error so it works on both scenarios I outlined above, via CDK and direct via node binary

there changelog also mentions this

My thoughts are, if you move to the /min import, everything should go back to normal and you can take some more time with the lru.min transition

sidorares · 2024-09-11T02:53:23Z

That said, as MySQL2 is mainly maintained by @sidorares, I would like at least a "LGTM" or an "approve" to merge it.

lets go the #2988 route :)

wellwelwel · 2024-09-11T03:27:43Z

Closing in order to #2988.

@robert-pitt-foodhub, thanks again 🙋🏻‍♂️

robert-pitt-foodhub · 2024-09-11T14:34:52Z

Nope Thank you @wellwelwel, I have spent the last 7 days/nights in the weeds of the mysql connection issue, and my goal was to smash the bug so that it's fixed once and for all and I really appreciate your responsiveness around this issues, it's been great working with you @wellwelwel, hopefully I will be back with some more fixes if I come across any scenarios that need addressing.

Thanks agian guys 🤝

Resolve ESM/CJS Module import conflict by using the minified lru-cach…

63abf71

…e export.

wellwelwel closed this Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve ESM/CJS Module import conflict by using the minified lru-cache #3038

Resolve ESM/CJS Module import conflict by using the minified lru-cache #3038

robert-pitt-foodhub commented Sep 10, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading

wellwelwel commented Sep 11, 2024

wellwelwel commented Sep 11, 2024 •

edited

Loading

robert-pitt-foodhub commented Sep 11, 2024

wellwelwel commented Sep 11, 2024 •

edited

Loading

wellwelwel commented Sep 11, 2024 •

edited

Loading

wellwelwel commented Sep 11, 2024 •

edited

Loading

robert-pitt-foodhub commented Sep 11, 2024 •

edited

Loading

wellwelwel commented Sep 11, 2024

wellwelwel commented Sep 11, 2024 •

edited

Loading

robert-pitt-foodhub commented Sep 11, 2024 •

edited

Loading

wellwelwel commented Sep 11, 2024 •

edited

Loading

wellwelwel commented Sep 11, 2024 •

edited

Loading

robert-pitt-foodhub commented Sep 11, 2024

wellwelwel commented Sep 11, 2024

sidorares commented Sep 11, 2024

wellwelwel commented Sep 11, 2024

robert-pitt-foodhub commented Sep 11, 2024

Resolve ESM/CJS Module import conflict by using the minified lru-cache #3038

Resolve ESM/CJS Module import conflict by using the minified lru-cache #3038

Conversation

robert-pitt-foodhub commented Sep 10, 2024

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

wellwelwel commented Sep 11, 2024

wellwelwel commented Sep 11, 2024 • edited Loading

robert-pitt-foodhub commented Sep 11, 2024

wellwelwel commented Sep 11, 2024 • edited Loading

"Why doesn't everyone upgrade to the latest version of lru-cache?"

wellwelwel commented Sep 11, 2024 • edited Loading

wellwelwel commented Sep 11, 2024 • edited Loading

robert-pitt-foodhub commented Sep 11, 2024 • edited Loading

wellwelwel commented Sep 11, 2024

wellwelwel commented Sep 11, 2024 • edited Loading

robert-pitt-foodhub commented Sep 11, 2024 • edited Loading

wellwelwel commented Sep 11, 2024 • edited Loading

wellwelwel commented Sep 11, 2024 • edited Loading

robert-pitt-foodhub commented Sep 11, 2024

wellwelwel commented Sep 11, 2024

sidorares commented Sep 11, 2024

wellwelwel commented Sep 11, 2024

robert-pitt-foodhub commented Sep 11, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading

wellwelwel commented Sep 11, 2024 •

edited

Loading

wellwelwel commented Sep 11, 2024 •

edited

Loading

"Why doesn't everyone upgrade to the latest version of `lru-cache`?"

wellwelwel commented Sep 11, 2024 •

edited

Loading

wellwelwel commented Sep 11, 2024 •

edited

Loading

robert-pitt-foodhub commented Sep 11, 2024 •

edited

Loading

wellwelwel commented Sep 11, 2024 •

edited

Loading

robert-pitt-foodhub commented Sep 11, 2024 •

edited

Loading

wellwelwel commented Sep 11, 2024 •

edited

Loading

wellwelwel commented Sep 11, 2024 •

edited

Loading