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

StubArray workaround is incompatible with terser's unsafe_arrows option #3825

Open
bradzacher opened this issue Feb 6, 2024 · 4 comments
Open
Labels

Comments

@bradzacher
Copy link

Intended outcome:

MobX should work with terser's unsafe_arrows optimisation option.

Actual outcome:

TypeError: Object.setPrototypeOf called on null or undefined

Info

// Typescript workaround to make sure ObservableArray extends Array
class StubArray {}
function inherit(ctor, proto) {
if (Object.setPrototypeOf) {
Object.setPrototypeOf(ctor.prototype, proto)
} else if (ctor.prototype.__proto__ !== undefined) {
ctor.prototype.__proto__ = proto
} else {
ctor.prototype = proto
}
}
inherit(StubArray, Array.prototype)

The mobx codebases utilises an empty class here.
This empty class is downlevelled by mobx's build process to be var StubArray = function StubArray() {}; (unpkg).

When terser runs with the [unsafe_arrows](https://terser.org/docs/options/#:~:text=unsafe_arrows%20(default%3A,2015%20or%20greater.) option it will convert all functions that don't reference this to an arrow, converting the above line to var StubArray = () => {}. This then breaks the inherit function which assumes StubArray.proto is defined.

Note: unsafe_arrows is (by definition) an unsafe optimisation - though in my experience it is generally safe because it's so rare to do operations on a function's prototype (outside of old-school "class" code).

It would be great if this line could be changed to make mobx more minifyable.

@urugator
Copy link
Collaborator

Do you have an idea for workaround?

@bradzacher
Copy link
Author

It's hard to say. TBH I'm not exactly sure why this workaround even exists!
I can see it was added in a9b5ed2 but the commit has no attached PR nor does it have any useful commit message.

In that commit I can see that there used to be some code that mutated the prototype - but that code is long since gone -- in v6 from the looks of it based on this comment:

// Weex proto freeze protection was here,
// but it is unclear why the hack is need as MobX never changed the prototype
// anyway, so removed it in V6

Seems to me like this code could just be class LegacyObservableArray<T> extends Array?

@bradzacher
Copy link
Author

It also seems odd to me that MobX's build process downlevels classes to functions since all browser have had support since around 2016.

@mweststrate
Copy link
Member

This workaround is not fixable I think as the arrow functions cannot act as base classes. The reason this downlevels this way is IIRC because when proxies are not used, this was the only way to reasonably correct inherit from the built-in array. If LegacyArray extends Array works nowadays, happy approve a PR for that!

I know it was a limitation in the past, and disabling proxies and using legacy array in the first place, is probably to target something from the past :) (Will be removed in next major)

@mweststrate mweststrate mentioned this issue Mar 9, 2024
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants