medikoo / memoizee

Complete memoize/cache solution for JavaScript
ISC License
1.75k stars 61 forks source link

bun compat issue #144

Closed RokoTechnology closed 1 month ago

RokoTechnology commented 2 months ago

Latest bun (1.1.26) will throw on this example:

const aaa = 'test'
aaa.promise = 'whatever'

image

This is what happens in latest memoizee (0.4.17) memoizee/ext/promise.js where you require "use strict" as string from memoizee/lib/registered-extensions.js and then try to set .promise on it: image image

medikoo commented 2 months ago

@RokoTechnology it must be issue with your tooling, note it's perfectly working JavaScript (well covered by tests: https://github.com/medikoo/memoizee/actions/runs/9220608521)

Maybe there's some bug with type definitions but they're not maintained here and not by me. I guess you need to investigat what exactly you're using

RuiNtD commented 1 month ago

I also get this error in runtime using Bun v1.1.30. This isn't a typing error. tsc compiles just fine. This is a runtime error.

memoize(_getListening, { maxAge: 5_000 })
11 |   , timeout    = require("timers-ext/valid-timeout")                                                         
12 |   , extensions = require("../lib/registered-extensions");                                                    
13 |
14 | var noop = Function.prototype, max = Math.max, min = Math.min, create = Object.create;
15 |
16 | extensions.maxAge = function (maxAge, conf, options) {
     ^
TypeError: Attempted to assign to readonly property.
      at Z:\#myprojs\lastfm-rp\node_modules\memoizee\ext\max-age.js:16:1
      at Z:\#myprojs\lastfm-rp\node_modules\memoizee\index.js:29:22
      at Z:\#myprojs\lastfm-rp\src\listenProvider\lastFm.ts:138:17
medikoo commented 1 month ago

@RuiNtD this is not a problem with this package, but with a tooling you're using. Have you tried to report it to Bun? Or any other utility that's responsible?

RuiNtD commented 1 month ago

I'm not familiar with the code of this package, so I don't even know what the issue is to report to Bun.

medikoo commented 1 month ago

@RuiNtD same what you reported here

medikoo commented 1 month ago

I'm closing this, as again there's no issue with this package, to confirm just try it in plain Node.js + npm

medikoo commented 1 month ago

Adding two cents. It appears that Bun makes wrong assumptions about JavaScript.

e.g. following is perfectly valid javascript:

const foo = {};
bar.prop = "elo";

If I read correctly it'll fail in Bun, because it comes up with assumption that foo is immutable object, which in JavaScript it isn't.

It might be that it's simply not compatible with plain JavaScript, but I guess there could be some option to make it work

heimskr commented 2 weeks ago

e.g. following is perfectly valid javascript:

const foo = {};
bar.prop = "elo";

This isn't valid JS because you're trying to access a property of bar, which was never declared or defined in this snippet. If you replace bar.prop with foo.prop, it works fine both in Bun 1.1.26 and in the latest version.

$ bunx bun@1.1.26 --print "const foo = {}; foo.prop = 'elo'; foo.prop"
elo
heimskr commented 2 weeks ago

The specific issue in memoizee is due to how Bun determines whether a module imported via require is a CommonJS module or an ES module. Currently, if you require a .js file that either is empty or contains "use strict" (as is the case for lib/registered-extensions.js), Node will consider it a CommonJS module and Bun's heuristics will decide it's an ES module. This makes a difference because CommonJS modules imported via require aren't frozen or sealed, but ES modules imported via require are. Therefore, in this case, Node will let you add properties to the module object and Bun won't. (The behavior differs slightly if you import a module via import. In that case, Bun and Node will treat an ES module as frozen and sealed and a CommonJS module as sealed but not frozen. This detail isn't relevant to this issue, though.)

Ideally, Bun's heuristics should, if the current version decides a module is ambiguous, check for "use strict" at global scope and declare the module to be a CommonJS module if it's found. This is because ES modules are in strict mode by default, so there's no reason to add an explicit "use strict", but "use strict" at global scope is perfectly sensible in a CommonJS module. If we change the heuristic as described in a future version of Bun, it will fix this issue.

medikoo commented 1 week ago

@heimskr, yes, my example was meant to be be:

const foo = {};
foo.prop = "elo";

Sorry for confusion.

Again, as you've noticed, this is perfectly working package (with millions of downloads, working well for 10+ years).

It's bun repository where discussion on this issue should be directed