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

Issue: Cannot assign to read only property 'length' of function #1238

Closed
Obi-Dann opened this issue Nov 13, 2017 · 7 comments
Closed

Issue: Cannot assign to read only property 'length' of function #1238

Obi-Dann opened this issue Nov 13, 2017 · 7 comments

Comments

@Obi-Dann
Copy link
Contributor

Obi-Dann commented Nov 13, 2017

We have a problem happening with 92 users (it's way less than 1% of our users)

The error is Cannot assign to read only property 'length' of function
at IObservableFactories line 1935, column 30 (webpack:///./node_modules/mobx/lib/mobx.js:1935)
image

It maps to line 195 in the following snippet

mobx/src/api/observable.ts

Lines 191 to 195 in b7fef45

// weird trick to keep our typings nicely with our funcs, and still extend the observable function
// ES6 class methods aren't enumerable, can't use Object.keys
Object.getOwnPropertyNames(IObservableFactories.prototype)
.filter(name => name !== "constructor")
.forEach(name => (observable[name] = IObservableFactories.prototype[name]))

Somehow, IObservableFactories.prototype have a property called length. Attempting to assign observable['length'] fails in strict mode, because length is a read-only property.

I found no way to reproduce it. I am really keen on blaming browser extensions, but this issue happens across multiple browsers:

4 browsers affected %
Chrome 61%
Mobile Safari 35%
IE 2%
Edge 2%

I guess the only idea I have is be wrap the code assigning props into try{} catch(e){} or to check filter out non-writable properties.

I don't mind creating a PR, but it'd be nice to know what you guys think and what approach is preferable before I do it

@Obi-Dann
Copy link
Contributor Author

Obi-Dann commented Nov 15, 2017

Perhaps, the PR I posted is not going to solve the issue, we've just had another similar error report in environment with the proposed fix applied
image

Going to investigate deeper

@Obi-Dann
Copy link
Contributor Author

Weird, IObservableFactories.prototype must have a value of some kind of rubbish object but it does not make sense how that value gets assigned...

@mweststrate
Copy link
Member

@DanNSam I would be fine by introducing a simple workaround, that filters out length as well, or just make everythign explicit, kill the loop, and copy the relevant functions by hand. Interested in changing your PR to that if that solves the issue?

@Obi-Dann
Copy link
Contributor Author

Thanks @mweststrate I'll check it out and change my PR if it solves the issue!

@Obi-Dann
Copy link
Contributor Author

I am going to trial these changes df5002c over the weekend and create a PR if the issue is resolved

@Obi-Dann
Copy link
Contributor Author

Obi-Dann commented Nov 26, 2017

Trialed changes from #1257 over the weekend. There were no new error reports, I think it fixes the issue. @mweststrate, could you please review and merge #1257 if it looks good to you? I wonder if it's acceptable to remove IObservableProperties from API

The only reason, I can think of, why the issue originally occurred is a browser extension polyfilling/mutating Object. getOwnPropertyNames, but it's a wild guess.

@mweststrate
Copy link
Member

Released as 3.3.3. Thanks for the investigation and PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants