Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

A szn label called "clear" breaks since integrating collections #1084

Closed
Stuk opened this Issue · 14 comments

3 participants

@Stuk
Collaborator

5c596c4 made any labels called "clear" in the serialization break. This can be seen on HEAD of mfiddle, when you expand the console to reveal the Clear button.

@kriskowal @aadsm

@aadsm aadsm was assigned
@aadsm aadsm referenced this issue from a commit in aadsm/montage
@aadsm aadsm [gh-1084] Serializer switch to Object.create(null)
Collections added specific properties to Object.prototype that made the (de)serializer brake, a change to Object.create(null) fixes all those issues.
07d041d
@aadsm
Collaborator

Can we close this one?

@Stuk
Collaborator

Afraid not. I'm really sorry, I should have checked this issue was solved by your PR before merging.

The instances object used here: https://github.com/montagejs/montage/blob/master/core/deserializer.js#L1106 and then throughout the deserializer actually comes from here: https://github.com/montagejs/montage/blob/master/ui/component.js#L946 and so clear is still on the prototype.

This case is a pretty simple fix, but I'm now worried there are going to be more gotchas later...

@aadsm
Collaborator

Well spot, I think I'll just copy the properties over then, shouldn't trust the input.

@Stuk
Collaborator

You could just set __proto__ to null :) And perhaps this should be done for any public method that accepts instances

@aadsm
Collaborator

yeah, but that wouldn't work on IE :(

@Stuk Stuk referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@aadsm
Collaborator

Can we close this one?

@Stuk
Collaborator

Still seems to be broken on montage master. See the Clear button on the console in mfiddle

@Stuk
Collaborator

Will this be solved by mousse?

@aadsm
Collaborator

I believe this problem won't exist anymore after FRB because we'll migrate from Object.prototype.clear to Object.clear, however, I'll make sure there's a test for that.

@mczepiel
Collaborator

Any updates on this? (Feel free to close, I'm just spending some time trying to clear up old bugs)

@aadsm
Collaborator
@aadsm
Collaborator

Same issue but in another part of the code, it is fixed now!

@aadsm aadsm closed this
@Stuk
Collaborator

:tada:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.