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

Adding a method to Object.prototype causes error on ports #710

Closed
EuAndreh opened this Issue Sep 12, 2016 · 3 comments

Comments

Projects
None yet
4 participants
@EuAndreh

EuAndreh commented Sep 12, 2016

Whenever you try to add any functionality to Object.prototype, Elm ports throws an error and stops working.

This is a small reproducible example:

import Html
import Html.App

main = Html.App.beginnerProgram { model = model, view = view, update = update }

model = 0

type Msg = Nop
update msg model =
  case msg of
    Nop ->
      model

view model =
  Html.div [] []

Run:

$ elm make src/Main.elm --output=index.js

index.html file:

<html>
  <body>
    <div>
      <script src="index.js"></script>
      <script>
        var app = Elm.Main.fullscreen();     // embed or fullscreen are just the same
        Object.prototype.whatever = function () {};  // <--- this is the problem
      </script>
    </div>
  </body>
</html>

Served with:

$ python -m SimpleHTTPServer 3000

It says:

index.js:2800 Uncaught TypeError: Cannot read property 'push' of undefined

And if the Object.prototype.whatever line gets commented, everything goes back to working normally.

After the error, ports.send stop working.

  • Chrome 52
  • macOS El Capitan
  • Elm 0.17.1
@process-bot

This comment has been minimized.

Show comment
Hide comment
@process-bot

process-bot Sep 12, 2016

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot commented Sep 12, 2016

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@lukewestby

This comment has been minimized.

Show comment
Hide comment
@lukewestby

lukewestby Sep 12, 2016

Member

Some investigation notes:

Adding a property like this to Object.prototype is going to create a property on all objects that isn't an own property but is still enumerable. As a result, any time for .. in is used, including at the location of this error (https://github.com/elm-lang/core/blob/master/src/Native/Platform.js#L342), an enumerable property on Object.prototype is going to cause problems.

If this is something we want to fix here are a couple options with different tradeoffs:

  1. If an object in core is intended to be used as a dictionary or bag of some kind, create it with Object.create(null). This creates a prototype-less object with no properties but its own.
    • Pro: no added performance cost during iteration, as the for .. ins can stay how they are
    • Con: No support for IE8
  2. Guard every iteration of a for .. in loop with hasOwnProperty().
    • Pro: perfect browser support
    • Con: has to be added in a lot of places and adds an additional function call to every iteration over an object's properties
Member

lukewestby commented Sep 12, 2016

Some investigation notes:

Adding a property like this to Object.prototype is going to create a property on all objects that isn't an own property but is still enumerable. As a result, any time for .. in is used, including at the location of this error (https://github.com/elm-lang/core/blob/master/src/Native/Platform.js#L342), an enumerable property on Object.prototype is going to cause problems.

If this is something we want to fix here are a couple options with different tradeoffs:

  1. If an object in core is intended to be used as a dictionary or bag of some kind, create it with Object.create(null). This creates a prototype-less object with no properties but its own.
    • Pro: no added performance cost during iteration, as the for .. ins can stay how they are
    • Con: No support for IE8
  2. Guard every iteration of a for .. in loop with hasOwnProperty().
    • Pro: perfect browser support
    • Con: has to be added in a lot of places and adds an additional function call to every iteration over an object's properties

@lukewestby lukewestby added the bug label Sep 12, 2016

@halfzebra

This comment has been minimized.

Show comment
Hide comment
@halfzebra

halfzebra Sep 26, 2016

Contributor

@lukewestby it must be possible to shim that with something like zloirock/core-js

Object.create(null) sounds like a better way to me.

Contributor

halfzebra commented Sep 26, 2016

@lukewestby it must be possible to shim that with something like zloirock/core-js

Object.create(null) sounds like a better way to me.

@EuAndreh EuAndreh closed this Feb 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment